Reentrant add node to network #707
Conversation
WalkthroughThe changes modify the error handling in the Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant NodeHandler
participant Logger
Caller->>NodeHandler: Request to add node to network
NodeHandler-->>NodeHandler: Attempt node addition
alt Error contains "FailedToAddNodeToCanonicalNetwork"
NodeHandler->>Logger: Log informational message ("node is already in the network")
else Other Error Occurred
NodeHandler->>Logger: Log fatal error
end
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (1.64.8)Error: you are using a configuration file for golangci-lint v2 with golangci-lint v1: please use golangci-lint v2 ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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 (1)
cmd/cli/main.go (1)
272-277: Good error handling improvement for reentrant function calls.The updated error handling makes the function reentrant by treating "node already in network" as a non-fatal condition. This matches the PR objective of making the add node function reentrant.
However, string matching for error classification is fragile and could break if error messages change.
// TODO(borja): This is a patch until NodeRegistry implements fine grain errors. if strings.Contains(err.Error(), "FailedToAddNodeToCanonicalNetwork") { logger.Info("node already in network") } else { logger.Fatal("could not add node to network", zap.Error(err)) }Consider enhancing the TODO comment to include a tracking issue number for implementing proper error types in NodeRegistry, which would allow for more robust error checking using
errors.Is()or error code comparisons instead of string matching.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/cli/main.go(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Push Docker Images to GitHub Packages (xmtpd-cli)
- GitHub Check: Upgrade Tests
- GitHub Check: Push Docker Images to GitHub Packages (xmtpd)
- GitHub Check: Build pre-baked anvil-xmtpd
- GitHub Check: Test (Node)
🔇 Additional comments (1)
cmd/cli/main.go (1)
10-10: LGTM: Import added for string matching.The
stringspackage is appropriately added to support the new error handling logic.
Modify
addNodeToNetworkfunction in CLI to handle duplicate node additions gracefullyUpdates error handling in main.go to prevent fatal errors when adding nodes that already exist in the network:
FailedToAddNodeToCanonicalNetworkerror string📍Where to Start
Start with the
addNodeToNetworkfunction in main.go where the error handling logic has been modified.Macroscope summarized 8011bc8.
Summary by CodeRabbit