feat: Enable auto memory configuration by default#8047
Closed
Conversation
This change enables the `advanced.autoConfigureMemory` setting by default to help prevent out-of-memory errors. Key changes: - The default value for `advanced.autoConfigureMemory` is now `true`. - Added telemetry to record memory usage (max old space size, total heap size, used heap size) and OOM errors. - Updated documentation to reflect the new default and added a troubleshooting section for memory-related questions. - Added an integration test to verify that the `--max-old-space-size` flag is correctly applied.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Summary of Changes
Hello @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 stability of the Gemini CLI by enabling automatic memory configuration by default, aiming to reduce out-of-memory errors. It also introduces comprehensive telemetry to monitor memory usage and OOM events, alongside updated documentation to guide users. These changes collectively improve the user experience by making the CLI more robust and providing better diagnostic capabilities.
Highlights
- Default Memory Configuration: The
advanced.autoConfigureMemorysetting is now enabled by default to proactively manage Node.js memory limits and help prevent out-of-memory (OOM) errors. - Telemetry for Memory Usage: New telemetry metrics have been introduced to record memory usage (max old space size, total heap size, used heap size) and OOM error counts, providing better insights into application stability.
- Improved OOM Error Handling: A process-level
uncaughtExceptionhandler has been added to specifically catch and log OOM errors, enhancing error reporting. - Documentation Updates: The CLI configuration documentation has been updated to reflect the new default setting, and a new troubleshooting entry addresses common questions about memory consumption.
- New Integration and Unit Tests: A new integration test verifies that the
--max-old-space-sizeflag is correctly applied, and a unit test confirms the new default setting foradvanced.autoConfigureMemory.
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
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
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
|
Size Change: +2.24 kB (+0.02%) Total Size: 13.2 MB
ℹ️ View Unchanged
|
gemini-cli
bot
added
kind/enhancement
priority/p2
Contributor
There was a problem hiding this comment.
Code Review
This pull request is a great initiative to improve the stability of the CLI by enabling automatic memory configuration by default. The addition of telemetry for memory usage and OOM errors is a valuable enhancement for monitoring the feature's impact. The updated documentation and new tests are also welcome additions.
My review focuses on a few key areas to ensure correctness and robustness:
- There's a unit mismatch in the memory telemetry, where a value in megabytes is recorded for a metric expecting bytes.
- The new test for the default memory setting doesn't correctly verify the default behavior.
- The
uncaughtExceptionhandler could be made safer by avoiding re-throwing the error.
These are important points to address to ensure the feature is reliable and the codebase remains maintainable.
Comment on lines
+124
to
+139
| it('should have advanced.autoConfigureMemory as true by default', () => { | ||
| (mockFsExistsSync as Mock).mockImplementation( | ||
| (p: fs.PathLike) => p === USER_SETTINGS_PATH, | ||
| ); | ||
| const userSettingsContent = { advanced: { autoConfigureMemory: true } }; | ||
| (fs.readFileSync as Mock).mockImplementation( | ||
| (p: fs.PathOrFileDescriptor) => { | ||
| if (p === USER_SETTINGS_PATH) | ||
| return JSON.stringify(userSettingsContent); | ||
| return '{}'; | ||
| }, | ||
| ); | ||
|
|
||
| const settings = loadSettings(MOCK_WORKSPACE_DIR); | ||
| expect(settings.merged.advanced?.autoConfigureMemory).toBe(true); | ||
| }); |
Contributor
There was a problem hiding this comment.
This test is titled 'should have advanced.autoConfigureMemory as true by default', but it doesn't test the default value. It currently tests that the value is loaded correctly when explicitly set to true in user settings.
To properly test the default value, the test should be structured to load settings where autoConfigureMemory is not defined. The suggested change does this by simulating no settings files.
Note that this corrected test may fail if loadSettings does not apply defaults from the schema (which seems to be implied by the should load empty settings if no files exist test). If it fails, it indicates that the test for the default value should be added at a higher level where defaults are actually applied.
it('should have advanced.autoConfigureMemory as true by default', () => {
(mockFsExistsSync as Mock).mockReturnValue(false);
(fs.readFileSync as Mock).mockReturnValue('{}');
const settings = loadSettings(MOCK_WORKSPACE_DIR);
expect(settings.merged.advanced?.autoConfigureMemory).toBe(true);
});
Comment on lines
+93
to
+98
| recordMemoryMetrics( | ||
| config, | ||
| targetMaxOldSpaceSizeInMB, | ||
| heapStats.total_heap_size, | ||
| heapStats.used_heap_size, | ||
| ); |
Contributor
There was a problem hiding this comment.
There's a unit mismatch in the telemetry data being recorded. The maxOldSpaceSizeHistogram metric is configured with the unit 'By' (bytes) in packages/core/src/telemetry/metrics.ts, but it's being passed the value targetMaxOldSpaceSizeInMB, which is in megabytes. The other memory metrics (total_heap_size, used_heap_size) are correctly reported in bytes.
To ensure data consistency and correctness, you should convert the targetMaxOldSpaceSizeInMB value to bytes before passing it to recordMemoryMetrics.
Suggested change
| recordMemoryMetrics( | |
| config, | |
| targetMaxOldSpaceSizeInMB, | |
| heapStats.total_heap_size, | |
| heapStats.used_heap_size, | |
| ); | |
| recordMemoryMetrics( | |
| config, | |
| targetMaxOldSpaceSizeInMB * 1024 * 1024, | |
| heapStats.total_heap_size, | |
| heapStats.used_heap_size | |
| ); |
| if (err.name === 'Error' && err.message.includes('out of memory')) { | ||
| recordOomError(config); | ||
| } | ||
| throw err; |
Contributor
There was a problem hiding this comment.
While re-throwing the error in an uncaughtException handler is a way to ensure the process terminates, it's generally considered safer and more explicit to log the error and then call process.exit(1). The process is in an unstable state after an uncaught exception, and re-throwing can have subtle side effects. Explicitly exiting provides a more predictable shutdown sequence.
console.error('Uncaught Exception:', err);
process.exit(1);
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
jacob314
reviewed
Sep 9, 2025
| ); | ||
| } | ||
|
|
||
| recordMemoryMetrics( |
Contributor
There was a problem hiding this comment.
memory usage on startup isn't that useful. What is relevant is peak usage overall over the course of running the app. There is a different PR that generally takes the right path for this although it has a lot of other functionality as well.
#2157 (review)
Contributor
There was a problem hiding this comment.
That's a very good point, jacob314. While capturing peak memory usage over the application's lifetime is indeed more comprehensive and ultimately more indicative of overall resource consumption, recording the initial memory usage at startup still provides valuable baseline data. This helps us understand the initial footprint of the CLI and identify any potential startup overhead.
This PR primarily focuses on enabling the autoConfigureMemory feature by default and establishing foundational telemetry for memory-related events, including OOM errors. Tracking peak usage would be a valuable enhancement, and I appreciate you pointing to PR #2157 as a reference for more advanced memory profiling. We can definitely consider integrating more sophisticated memory tracking in a follow-up effort.
gemini-cli
bot
added
status/need-issue
and removed
priority/p2
jacob314
deleted the
galzahavi/add/enableAutoConfigureMaxOldSpaceSize
branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TLDR
This pull request enables the
advanced.autoConfigureMemorysetting by default to help prevent out-of-memory errors. It also adds telemetry to record memory usage and OOM errors, updates documentation, and includes a new integration test to verify the functionality.Dive Deeper
The core of this change is setting the default value of
advanced.autoConfigureMemorytotrueinpackages/cli/src/config/settingsSchema.ts. This proactive memory management is intended to improve stability for users by default.To support and monitor this feature, the following changes were made:
packages/core/src/telemetry/to track memory-related data:METRIC_MAX_OLD_SPACE_SIZEMETRIC_TOTAL_HEAP_SIZEMETRIC_USED_HEAP_SIZEMETRIC_OOM_ERROR_COUNTpackages/cli/src/gemini.tsx) now includes a process-leveluncaughtExceptionhandler to specifically catch and log OOM errors.docs/cli/configuration.mdhas been updated to reflect the new default. A new entry indocs/troubleshooting.mdhas been added to address potential user questions about memory usage.integration-tests/auto_configure_memory.test.ts) has been created to ensure the--max-old-space-sizeflag is correctly passed to the underlying Node.js process.packages/cli/src/config/settings.test.tswas added to verify the new default setting.Reviewer Test Plan
npm run preflightto ensure all existing and new tests pass.ps aux | grep nodein another terminal to inspect the running Gemini CLI process and confirm that the--max-old-space-sizeflag is set.~/.gemini/settings.jsonfile and setadvanced.autoConfigureMemorytofalse.--max-old-space-sizeflag is no longer present in the process list.Testing Matrix
Linked issues / bugs
Fixes #7326