You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
I think we can keep this PR open and use this as a feature branch (meaning the other PRs from tickets from the same epic would merge into this instead of main), just to keep the feature scoped until we get more things going around it.
Memory Inefficiency 🟠 [major]
The readDebugLog function returns a string, which forces a full copy of the log data from the initial []byte buffer. Subsequently, doctorEntryPoint converts this string back into a []byte when calling workflow.NewData, creating yet another copy. Snyk debug logs can be very large; this triple-buffering approach (byte slice -> string -> byte slice) significantly increases memory pressure and the risk of Out-Of-Memory (OOM) failures for no functional benefit. The code should pass []byte throughout.
The readDebugLog function returns a string, which necessitates converting the []byte returned by os.ReadFile or io.ReadAll into a new string allocation (lines 76, 86). Subsequently, doctorEntryPoint converts this string back into a []byte at line 60 to pass it to workflow.NewData. Since Snyk debug logs can be extremely large, these redundant conversions significantly increase memory pressure by holding multiple copies of the data simultaneously. Returning []byte directly from readDebugLog would be more efficient.
[]byte(debugLog),
workflow.WithLogger(logger),
workflow.WithConfiguration(config),
)
return []workflow.Data{outputData}, nil
}
funcreadDebugLog(stdinio.Reader, inputPathstring) (string, error) {
ifinputPath!="" {
content, err:=os.ReadFile(inputPath)
iferr!=nil {
return"", cli.NewGeneralCLIFailureError(
fmt.Sprintf("Could not read the debug log file at '%s'. Check that the path is correct and the file is readable.", inputPath),
snyk_errors.WithCause(err),
)
}
returnstring(content), nil
}
content, err:=io.ReadAll(stdin)
iferr!=nil {
return"", cli.NewGeneralCLIFailureError(
"Could not read the debug log from standard input.",
snyk_errors.WithCause(err),
)
}
returnstring(content), nil
📚 Repository Context Analyzed
This review considered 12 relevant code sections from 11 files (average relevance: 0.85)
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
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.
Description
Checklist
make test)make generate)make lint)For the reviewer
I think we can keep this PR open and use this as a feature branch (meaning the other PRs from tickets from the same epic would merge into this instead of main), just to keep the feature scoped until we get more things going around it.