Conversation
There was a problem hiding this comment.
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 | ||
| } |
There was a problem hiding this comment.
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 | ||
| } |
There was a problem hiding this comment.
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.
SamMorrowDrums
left a comment
There was a problem hiding this comment.
Test might preserve motivation for this better and ensure regressions are deliberate.
Otherwise no comments.
|
🚀 |
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.