fix(core): address memory leaks in oauth flow, chat history, and shell parsing#24963
fix(core): address memory leaks in oauth flow, chat history, and shell parsing#24963spencer426 wants to merge 5 commits intomainfrom
Conversation
|
Hi @spencer426, thank you so much for your contribution to Gemini CLI! We really appreciate the time and effort you've put into this. We're making some updates to our contribution process to improve how we track and review changes. Please take a moment to review our recent discussion post: Improving Our Contribution Process & Introducing New Guidelines. Key Update: Starting January 26, 2026, the Gemini CLI project will require all pull requests to be associated with an existing issue. Any pull requests not linked to an issue by that date will be automatically closed. Thank you for your understanding and for being a part of our community! |
|
Size Change: +1.1 kB (0%) Total Size: 34 MB
ℹ️ View Unchanged
|
gemini-cli
Bot
added
the
status/need-issue
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 significantly improves memory management and application stability by addressing several distinct memory leaks. It targets issues ranging from uncleared timeouts in the OAuth flow and unbounded growth in chat history to hanging promises during Tree-sitter initialization and stale React closures. The changes collectively eliminate sources of excessive memory consumption and resource retention, leading to a more robust and efficient application. 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. 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 implements several memory management and stability improvements across the CLI and core packages. Key changes include memoizing the resetFileSearchState hook to prevent unnecessary effect triggers, capping the UI history size to 1000 items, and ensuring that authentication timeouts are consistently cleared using finally blocks. Review feedback identifies that the history truncation should also be applied to the underlying ChatRecordingService using a standard LruCache and points out a memory leak in the shell parser initialization where the timeout was not cleared upon success.
| if (newHistory.length > 1000) { | ||
| return newHistory.slice(newHistory.length - 1000); | ||
| } |
There was a problem hiding this comment.
While this change limits the history in the UI state, it does not address the unbounded growth in the ChatRecordingService itself, which was identified as the source of the ~350MB leak. The recordMessage method continues to append to an internal array without truncation. To resolve this, the messages state should be managed using the repository's standard LruCache implementation to prevent memory issues and ensure bounded growth. When implementing truncation, prefer a simple approach over complex logic if the inaccuracy is trivial.
References
- Avoid module-level global variables for state like caches to prevent race conditions and memory issues. Use session-scoped or instance-scoped state and leverage standard cache implementations like LRUCache.
- For caching, use the existing LruCache dependency instead of clearing the entire cache or implementing a custom LRU policy.
- When implementing truncation logic, a simpler approach is preferred over a more complex one if the potential inaccuracy is trivial compared to the overall buffer size.
| let timerId: NodeJS.Timeout | undefined; | ||
| const timeoutPromise = new Promise<void>((_, reject) => { | ||
| timerId = setTimeout( | ||
| () => reject(new Error('Tree-sitter initialization timed out')), | ||
| 5000, | ||
| ); | ||
| }); | ||
| treeSitterInitialization = Promise.race([ | ||
| loadBashLanguage(), | ||
| timeoutPromise, |
There was a problem hiding this comment.
The setTimeout for tree-sitter initialization is not cleared if the operation succeeds, creating a memory leak. Also, the 5-second timeout might be too short for some environments, leading to premature failures. Furthermore, avoid using the treeSitterInitialization promise object itself to track operation state; use an explicit state variable instead.
let timeoutId: NodeJS.Timeout | undefined;
const timeoutPromise = new Promise<void>((_, reject) => {
timeoutId = setTimeout(
() => reject(new Error('Tree-sitter initialization timed out')),
5000,
);
});
treeSitterInitialization = Promise.race([
loadBashLanguage(),
timeoutPromise,
])
.finally(() => {
if (timeoutId) clearTimeout(timeoutId);
})
.catch((error) => {References
- Do not blindly apply short timeouts (like 5 seconds) to operations, as they may be too short and cause operations to abort prematurely.
- When managing the state of asynchronous operations, rely on an explicit state variable rather than checking for the existence of a promise object.
|
Closing this monolithic PR in favor of four separate atomic PRs to make review, testing, and potential rollback easier for maintainers:
|
1 participant
Summary
Addresses four distinct memory leaks identified in the memory trace, eliminating unbounded growth and closure retention issues that caused gigabytes of memory consumption over time.
Details
packages/core/src/code_assist/oauth2.tsandpackages/core/src/utils/oauth-flow.ts, 5-minute timeouts were being created and never cleared upon success. This forced the garbage collector to retain the overarching Generator loop, pending Promises, and AsyncLocalStorage telemetry spans for the full 5 minutes. The fix explicitly tracks the timeout and clears it using a.finally()block.ChatRecordingServicewas holding ~350MB on its own because thehistoryManagerarray grew indefinitely without any truncation mechanism. The fix enforces a hard limit of 1000 items via an LRU-style slice inuseHistoryManager.ts.treeSitterInitializationPromise inshell-utils.tscould hang indefinitely if the native WASM bindings failed to initialize, leaking the module execution context. The fix wraps it in an aggressive 5000msPromise.racetimeout that rejects safely on failure.useAtCompletion.ts,resetFileSearchStatecaptured an initial stale closure to the Ink/React FiberNodes and held it forever in the global workspace context event listener. Wrapping the function inuseCallbackensures a stable, non-stale closure.(Note: A claim in the memory trace about
vimHandleInputclosures leaking was verified to be a misdiagnosis, as the handlers are correctly deregistered.)Related Issues
How to Validate
Start a local OAuth callback server flow to verify the timeout is properly cleared immediately:
Create a file
test-timeout.ts:Execute the script:
Ensure the script prints the output and exits instantaneously without hanging.
Pre-Merge Checklist