-
Notifications
You must be signed in to change notification settings - Fork 3.2k
Fix ring buffer panic #1556
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?
Fix ring buffer panic #1556
Conversation
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.
Pull request overview
This PR introduces a safeguard to prevent excessive memory usage in the ProcessResponseAsRingBufferToEnd function by capping the maximum number of job log lines that can be processed at 100,000.
Key Changes:
- Added a validation check that limits
maxJobLogLinesto a maximum of 100,000 before allocating memory for the ring buffer
| if maxJobLogLines > 100000 { | ||
| maxJobLogLines = 100000 | ||
| } |
Copilot
AI
Dec 8, 2025
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 new capping behavior lacks test coverage. Consider adding a test case in a new pkg/buffer/buffer_test.go file to verify that:
- Values at or below 100,000 are not modified
- Values above 100,000 are capped to 100,000
- The function still works correctly when the cap is applied
This is particularly important since this safeguard was added to prevent a panic, and we should have tests to ensure it works as expected.
| if maxJobLogLines > 100000 { | ||
| maxJobLogLines = 100000 | ||
| } |
Copilot
AI
Dec 8, 2025
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 function documentation should be updated to reflect the new behavior where maxJobLogLines is capped at a maximum value. The documentation currently states that the function will retain "the last maxJobLogLines lines" but doesn't mention that values exceeding 100,000 will be silently reduced.
Consider updating the documentation to include:
// Parameters:
//
// httpResp: The HTTP response whose body will be read.
// maxJobLogLines: The maximum number of log lines to retain (capped at 100,000).And add a note in the function description explaining this safeguard.
This pull request introduces a safeguard to the
ProcessResponseAsRingBufferToEndfunction inpkg/buffer/buffer.goto prevent excessive memory usage by capping the maximum number of job log lines that can be processed.