chore: implement exit on error on the on_init stage #95
Conversation
Signed-off-by: Valery Piashchynski <[email protected]>
WalkthroughThe changes in this pull request introduce a new boolean field Changes
Assessment against linked issues
Warning Rate limit exceeded@rustatian has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 22 minutes and 2 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
config.go (2)
37-38: LGTM! Consider enhancing the documentation.The new
ExitOnErrorfield correctly implements the requested functionality. However, the documentation comment could be more descriptive.Consider expanding the documentation to:
-// ExitOnError defines if the RR should exit if the command fails +// ExitOnError controls whether RoadRunner should terminate if the on_init command fails. +// When true, the server will exit with an error if initialization fails, which is +// particularly useful in containerized environments for fail-fast behavior.
Line range hint
52-69: Add default value initialization for ExitOnError.The
InitDefaultsmethod should initialize theExitOnErrorfield to ensure consistent behavior when the configuration is not explicitly set.func (cfg *Config) InitDefaults() error { if len(cfg.Command) == 0 { return errors.Str("command should not be empty") } if cfg.Relay == "" { cfg.Relay = "pipes" } if cfg.OnInit != nil { if len(cfg.OnInit.Command) == 0 { return errors.Str("on_init command should not be empty") } if cfg.OnInit.ExecTimeout == 0 { cfg.OnInit.ExecTimeout = time.Minute } + + // Default to false to maintain backward compatibility + // with existing deployments + if !cfg.OnInit.ExitOnError { + cfg.OnInit.ExitOnError = false + } } return nil }init.go (3)
51-55: Consider enhancing error loggingThe error propagation logic is correct, but the error logging could be more descriptive.
Consider adding more context to the error log:
-b.log.Error("process wait", zap.Error(errW)) +b.log.Error("init command failed during execution", zap.String("command", b.cfg.Command[0]), zap.Error(errW))
63-65: Remove unnecessary empty lineThe error handling is correct, but there's an unnecessary empty line that affects readability.
return err - }
Line range hint
29-70: Well-structured implementation of exit-on-error featureThe implementation successfully addresses issue #2075 with a clean error propagation chain. The changes are minimal, focused, and maintain backward compatibility while adding the requested functionality.
A few architectural considerations:
- The error channel approach allows for future extensibility if more detailed error handling is needed
- The timeout handling remains robust with proper cleanup
- The implementation properly separates concerns between error detection and handling
Consider documenting the error handling behavior in the README.md or documentation, particularly:
- The conditions under which the server will exit
- The meaning of different error types that might be returned
- Examples of common error scenarios and how to handle them
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
go.work.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
config.go(1 hunks)init.go(2 hunks)plugin.go(1 hunks)
🔇 Additional comments (3)
init.go (2)
29-29: LGTM: Channel type change enables error propagation
The change from chan struct{} to buffered chan error is well-designed, allowing proper error propagation while preventing goroutine leaks.
68-70: LGTM: Error handling chain is complete
The error propagation and timer cleanup are implemented correctly. Let's verify the complete error handling flow through the codebase.
✅ Verification successful
Let me gather more information about the error handling flow by checking the complete implementation.
Let me check one more aspect of the error handling chain to ensure completeness.
Let me fix the command syntax and try one more time.
Error handling chain is properly implemented
The error handling implementation in the select statement is correct and consistent with the overall error handling pattern in the codebase. The changes properly:
- Stop the timer when receiving an error from stopCh
- Propagate errors through the error handling chain
- Handle process termination and cleanup appropriately
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify error handling implementation across the codebase
# Check for consistent error handling in related files
rg -A 5 "ExitOnError"
# Verify the error handling in the plugin
ast-grep --pattern 'func (p *Plugin) Serve() error {
$$$
if err := $x; err != nil {
$$$
return err
}
$$$
}'
Length of output: 583
Script:
#!/bin/bash
# Check the complete implementation of error handling in init.go and plugin.go
rg -A 10 "func \(p \*Plugin\) Init\(\)"
# Check for error channel usage
rg -A 5 "errCh :="
# Check stopCh usage across files
rg -A 5 "stopCh :="
Length of output: 3741
Script:
#!/bin/bash
# Check the complete implementation of the select statement and error handling in init.go
rg -B 5 -A 10 "select {" init.go
# Check for any other error handling patterns in init.go
ast-grep --pattern 'if err := $x; err != nil {
$$$
return err
}'
Length of output: 185
Script:
#!/bin/bash
# Check the select statement implementation
rg -B 5 -A 10 "^[[:space:]]*select[[:space:]]*{" init.go
# Check error handling in init.go
rg -B 3 -A 3 "if.*err.*!= nil" init.go
# Get the complete content of init.go to verify the error handling flow
cat init.go
Length of output: 3186
plugin.go (1)
105-109: 🛠️ Refactor suggestion
Implementation aligns with requirements but could benefit from improved error handling.
The implementation correctly addresses the feature request from issue #2075 by adding the ability to exit when on_init fails. However, there are a few suggestions to enhance the error handling:
Consider wrapping the error with additional context before propagating it:
if p.cfg.OnInit.ExitOnError {
- errCh <- err
+ errCh <- fmt.Errorf("on_init command failed: %w", err)
return errCh
}Let's verify the ExitOnError field is properly defined in the config struct:
Consider adding metrics or structured logging to track initialization failures. This would help monitor the frequency and nature of initialization issues in production environments, especially useful in Kubernetes deployments mentioned in the requirements.
✅ Verification successful
ExitOnError field is properly defined and documented, but error wrapping can be improved
The implementation correctly uses the ExitOnError field which is properly defined in the InitConfig struct with appropriate documentation. However, the error handling can be enhanced by wrapping the error with additional context as suggested:
if p.cfg.OnInit.ExitOnError {
- errCh <- err
+ errCh <- fmt.Errorf("on_init command failed: %w", err)
return errCh
}This change would provide better error context while maintaining the requested exit-on-error functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the ExitOnError field in InitConfig struct
ast-grep --pattern 'type InitConfig struct {
$$$
ExitOnError bool
$$$
}'
Length of output: 726
Signed-off-by: Valery Piashchynski <[email protected]>
Signed-off-by: Valery Piashchynski <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
tests/configs/.rr-on-init-error.yaml (1)
4-7: Consider reducing the exec_timeout for test scenariosThe
exec_timeoutof 200 seconds seems excessive for a test configuration, especially when testing error scenarios that should fail quickly.Consider reducing this to a more reasonable value like 5-10 seconds:
on_init: command: "foooooo" - exec_timeout: 200s + exec_timeout: 10s exit_on_error: true
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
tests/configs/.rr-on-init-error.yaml(1 hunks)tests/server_plugin_test.go(1 hunks)
🔇 Additional comments (4)
tests/configs/.rr-on-init-error.yaml (4)
1-2: LGTM: Version declaration is correct
The version specification follows the expected format.
11-13: LGTM: Logging configuration is appropriate for testing
The development mode and debug level are well-suited for testing error scenarios.
1-13: Consider file naming convention
The file name starts with a dot (.rr-on-init-error.yaml), which makes it a hidden file in Unix-like systems. While this might be intentional for test configurations, it could cause confusion or visibility issues.
3-8: Verify configuration structure against schema
Let's verify that the configuration structure matches RoadRunner's schema.
✅ Verification successful
Configuration structure is valid
The configuration structure in .rr-on-init-error.yaml matches the schema defined in config.go. The on_init section is properly structured with the following valid fields:
command: string arrayexec_timeout: durationexit_on_error: boolean
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for similar configuration structures in other test files
# and documentation to verify the correct format
# Check other test configurations
fd -e yaml -e yml . tests/configs -x grep -l "on_init"
# Check documentation
fd -e md . docs -x grep -l "on_init"
Length of output: 183
Script:
#!/bin/bash
# Let's try a broader search for configuration examples and documentation
# Search for any YAML files in the repository
fd -e yaml -e yml . -x grep -l "on_init"
# Search for configuration-related documentation using ripgrep
rg -l "on_init" --type-add 'docs:*.{md,rst,txt}' -tdocs
# Search for schema definitions or configuration examples
rg -l "on_init.*command" --type=yaml
Length of output: 135
Script:
#!/bin/bash
# Let's try to find the schema definition or configuration validation code
# Search for server configuration related code
rg -l "server.*config" --type=go -C 3
# Search for configuration validation or schema definition
ast-grep --pattern 'type ServerConfig struct {
$$$
}'
# Look for test files that might contain configuration examples
fd -e go -x grep -l "TestServer.*Config"
Length of output: 178
Script:
#!/bin/bash
# Let's examine the config.go file that was found
cat config.go
# Also check the test file for configuration examples
cat tests/server_plugin_test.go
# Look for any configuration validation code
rg "func.*validate" config.go -A 5
Length of output: 16345
Reason for This PR
closes: roadrunner-server/roadrunner#2075
Description of Changes
exit_on_errorto theon_initoptions.License Acceptance
By submitting this pull request, I confirm that my contribution is made under
the terms of the MIT license.
PR Checklist
[Author TODO: Meet these criteria.][Reviewer TODO: Verify that these criteria are met. Request changes if not]git commit -s).CHANGELOG.md.Summary by CodeRabbit
New Features
ExitOnErrorconfiguration option, allowing the application to terminate if a command fails.Bug Fixes
Tests
Chores