-
Notifications
You must be signed in to change notification settings - Fork 8
fix: Prevent nil pointer panic in monitor loop #24
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
| } else { | ||
| log.WithError(err).Errorf("failed to get machine %s: %v", job.MachineID.String, err) | ||
| } | ||
| log.WithError(err).Errorf("failed to get machine %s: %v", job.MachineID.String, err) |
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.
I'm not 100% sure if it deliberately proceeds in non-404 cases, so let me know if it does. 🙂
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 fixes a potential nil pointer dereference panic in the evaluateJob function by ensuring proper error handling when MachineGet fails and by adding a defensive nil check even when no error is returned.
Changes:
- Added early return when
MachineGetreturns a non-404 error to prevent further processing - Added explicit nil check for the machine object before accessing its properties
- Restructured error handling logic to use if-else for better control flow
Comments suppressed due to low confidence (1)
internal/cron/monitor.go:79
- The error is being logged with both
WithError(err)and%vformatting of the same error, which results in duplicate error information in the log output. Remove the: %vanderrparameter from the format string to avoid redundancy.
log.WithError(err).Errorf("failed to get machine %s: %v", job.MachineID.String, err)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
internal/cron/monitor.go
Outdated
| } | ||
| return nil | ||
| } else { | ||
| log.WithError(err).Errorf("failed to get machine %s: %v", job.MachineID.String, err) |
Copilot
AI
Jan 16, 2026
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 error is being logged with both WithError(err) and %v formatting of the same error, which results in duplicate error information in the log output. Remove the : %v and err parameter from the format string to avoid redundancy.
The
evaluateJobfunction was proceeding to process job events even whenMachineGetwas kind of unsuccessful; the exit code404alone is covered.changes:
returnwhenMachineGetreturns an error (non-404).nileven if no error is returned