chore: return error if the startup process was killed by timeout #103
Conversation
WalkthroughThis pull request updates the error handling logic in the Changes
Sequence Diagram(s)sequenceDiagram
participant User as User/Application
participant Cmd as Command.start()
participant Config as ExitOnError Flag
User->>Cmd: Invoke start()
Cmd->>Config: Check ExitOnError setting
alt ExitOnError is true
Cmd-->>User: Return error "startup process killed by timeout"
else
Cmd-->>User: Return nil
end
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
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: 0
🧹 Nitpick comments (2)
tests/server_plugin_test.go (1)
271-294: Consider adding specific error message assertions.While the test correctly verifies that an error occurs, it would be more robust to assert the specific error message expected when the startup process is killed by timeout.
_, err = cont.Serve() require.Error(t, err) + require.Contains(t, err.Error(), "startup process has been killed by timeout")tests/configs/.rr-on-init-error-timeout.yaml (1)
4-8: Add a comment explaining the test scenario.Consider adding a YAML comment to explain that this configuration is designed to test timeout error handling by intentionally setting a very short timeout (1ns) for a longer operation (sleep 1s).
server: + # Test configuration: Forces a timeout by setting 1ns timeout for a 1s sleep on_init: command: "sleep 1" exec_timeout: 1ns exit_on_error: true
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.work.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
init.go(1 hunks)tests/configs/.rr-on-init-error-timeout.yaml(1 hunks)tests/server_plugin_test.go(1 hunks)
🔇 Additional comments (1)
init.go (1)
66-70: LGTM! The error handling for timeout aligns with the PR objective.The implementation correctly returns an error when
ExitOnErroris true and maintains backward compatibility by returning nil otherwise.
AleksandrKosovS
force-pushed
the
master
branch
from
706eaaf to
1638cea
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
AleksandrKosovS
force-pushed
the
master
branch
from
1638cea to
df441d9
Compare
rustatian
changed the title
AleksandrKosovS
force-pushed
the
master
branch
from
df441d9 to
cc33c82
Compare
Reason for This PR
This feature has a minor discrepancy: if the initialization process is terminated due to a timeout, the server incorrectly considers it a successful execution.
Description of Changes
This patch returns an error if ExitOnError is enabled.
License Acceptance
By submitting this pull request, I confirm that my contribution is made under
the terms of the MIT license.
Change in the documentation
roadrunner-server/docs#47
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
Summary by CodeRabbit
Bug Fixes
New Features
Tests