-
Notifications
You must be signed in to change notification settings - Fork 1.7k
chore: add cleanup disk space step #9930
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Summary of ChangesHello @menahyouyeah, 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 primarily addresses a critical issue causing Linux integration tests to fail due to insufficient disk space in the GitHub workflow by adding a necessary cleanup step. Beyond this immediate fix, it includes a comprehensive update of several Highlights
Ignored Files
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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request updates several Go dependencies, which brings in a number of upstream fixes and improvements. These include security enhancements like added bounds checks in the SSH package and buffer overlap checks in chacha20poly1305, as well as performance optimizations such as gzip.Reader pooling in the HTTP/2 transport. Overall, these are valuable updates. However, I've identified a potential memory leak in the updated vendored HTTP/2 code that should be addressed.
| n.addBytes(-n.bytes) | ||
|
|
||
| q := n.q | ||
| ws.queuePool.put(&q) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The removal of n.q.s = nil (from the previous version of the code) without a replacement to clear the slices in n.q appears to introduce a memory leak. The writeQueue struct was refactored to use currQueue and nextQueue, but these are not being cleared on the priorityNode instance (n) which may be kept in ws.closedNodes.
The call to ws.queuePool.put(&q) operates on a copy of n.q, so n.q itself is not modified. To prevent the memory leak, the queue slices on n.q should be cleared after this line.
Suggested fix:
ws.queuePool.put(&q)
n.q.currQueue = nil
n.q.nextQueue = nil3ea94b6 to
7d585c0
Compare
Merge before/after: Dependent or prerequisite PRs
Merge before #9928
Description
Add clean up disk space step in the github workflow, otherwise the Linux integration tests fails with
panic.go:615: docker build failure: write /var/lib/docker/tmp/GetImageBlob864252763: no space left on device. Please fix the Dockerfile and try again..