Implement enhanced proxy management and logging in the daemon + restart logic throttling for MacOS and Windows #96
Conversation
- Introduced constants for proxy start retries and logging intervals. - Added logic to handle proxy start attempts with retry limits and intervals. - Implemented a logging mechanism for daemon status at defined intervals. - Updated the macOS plist to include a throttle interval for the daemon.
- Added a KeepAlive key to the plist to ensure the daemon remains active.
- Added Version method to the Scanner interface for better version management. - Implemented Version method in GitHookScanner and VSCodeScanner. - Updated SafechainScanner to use a context value for logging control during command execution. - Improved daemon logging to include installed scanners and their versions. - Added failure actions for the SafeChainUltimate service in the Windows installer.
tudor-timcu
changed the title
…andWithEnv function - Updated SafechainScanner to return "Safechain" instead of "safechain". - Enhanced logging control in RunCommandWithEnv to handle context value more robustly, ensuring logging is only performed when intended.
…ce failure actions - Added OnInstall="yes" to ServiceConfigFailureActions to ensure proper handling during installation.
- Added util namespace for improved service configuration. - Replaced ServiceConfigFailureActions with util:ServiceConfig for better failure action management during service installation.
- Included the WixToolset.Util.wixext extension in the build-msi.ps1 script to enhance functionality during the MSI build process.
- Updated the build-windows.yml workflow to include the WixToolset.Util.wixext extension alongside WixToolset.UI.wixext for enhanced MSI building capabilities.
- Increased RestartServiceDelayInSeconds from 60 to 300 to allow for a longer wait time before restarting the service after a failure.
- Updated the logStatus method in the Daemon to include the proxy version when it is running. - Improved the logging output for the proxy status to indicate when it is not running. - Added a Version method in the Proxy to retrieve the current version of the proxy.
![aikido-pr-checks[bot]](https://avatars.githubusercontent.com/in/898896?s=60&v=4){kind=small}
internal/daemon/daemon.go
Outdated
| } | ||
| } | ||
|
|
||
| func (d *Daemon) handleProxy() error { |
There was a problem hiding this comment.
Function 'handleProxy' groups retry throttling, state mutation, startup attempts, and logging; name is vague. Use a more descriptive name or split responsibilities.
Details
✨ AI Reasoning
The newly added method 'handleProxy' performs multiple actions: checks proxy status, enforces retry limits and intervals, mutates internal retry counters/timestamps, initiates proxy start attempts (calling startProxyAndInstallCA), and logs outcomes. Its name 'handleProxy' is generic and does not convey these distinct responsibilities (throttling, retry counting, startup). This makes the function's intent and responsibilities unclear to readers and maintainers. Splitting responsibilities or choosing a more descriptive name would make intent clearer.
🔧 How do I fix it?
Use descriptive verb-noun function names, add docstrings explaining the function's purpose, or provide meaningful return type hints.
Reply @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info
- Changed RestartServiceDelayInSeconds from 300 to 60 in the Windows installer. - Updated ThrottleInterval from 300 to 60 in the macOS plist file.
internal/daemon/daemon.go
Outdated
| } | ||
|
|
||
| if d.proxyRetryCount >= ProxyStartMaxRetries { | ||
| return fmt.Errorf("proxy start retry limit reached (%d attempts), not retrying", d.proxyRetryCount) |
There was a problem hiding this comment.
wouldn't we want to exit at this point instead of remaining in this loop?
There was a problem hiding this comment.
or errors being bubbled by heartbeat will cause an exit?, in that case let's rewrite a bit so all the errors in this function are returned and handled in heartbeat and lets do a filter on this error so that it's a clearer design choice we want to exit the program on this error
There was a problem hiding this comment.
wdym? a hearbeat that returns error will just cause the daemon to exit. so if handleProxy returns an error, the daemon exits. it returns an error only if retry count is exceeded.
- Renamed logStatus method to printDaemonStatus for clarity. - Updated heartbeat method to call printDaemonStatus. - Improved error handling in Proxy.Version method to check for empty output and parsing errors.
- Added a comment to clarify the behavior when the proxy start retry limit is reached, indicating that the daemon loop will exit in this case.
- Updated handleProxy method to return a boolean indicating whether to retry starting the proxy, along with an error if applicable. - Adjusted error handling in the heartbeat method to log failures without terminating the process.
internal/daemon/daemon.go
Outdated
| } | ||
|
|
||
| if !d.proxyLastRetryTime.IsZero() && time.Since(d.proxyLastRetryTime) < ProxyStartRetryInterval { | ||
| log.Printf("Proxy is not running, waiting for retry interval before next attempt") |
There was a problem hiding this comment.
could we not add the retry interval inside this print statement, seems like go handles stringifies of durations okay?
- Updated the log message in the handleProxy method to specify the retry interval duration when the proxy is not running, enhancing clarity for debugging and monitoring purposes.
….0.2 - Changed the WiX extension commands in the build-windows.yml file to specify version 6.0.2 for WixToolset.UI.wixext and WixToolset.Util.wixext, ensuring compatibility and stability in the build process.
2 participants
Summary by Aikido
⚡ Enhancements
🔧 Refactors
More info