feat: discord bot reconnection #2105
Conversation
…nge event reception logic
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
WalkthroughThe pull request includes significant updates to the Changes
Possibly related PRs
Suggested reviewers
Poem
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 (
|
|
Failed to generate code suggestions for PR |
![mentatbot[bot]](https://avatars.githubusercontent.com/in/837875?s=60&v=4){kind=small}
There was a problem hiding this comment.
The PR improves error handling and reconnection logic for the Discord bot, particularly around the Torii client subscription. The changes look good overall with proper error propagation and logging. A few suggestions for improvement:
- Consider making retry parameters configurable
- Add proper error handling for channel operations
- Consider adding shutdown mechanisms for spawned tasks
- The backoff logic is well implemented but could be extracted into a reusable utility
Overall this is a solid improvement to the bot's reliability.
Thanks for using MentatBot. Give comments a 👍 or 👎 to help me improve!
| @@ -50,47 +59,57 @@ impl ToriiClientSubscriber { | |||
| let mut backoff = Duration::from_secs(1); | |||
There was a problem hiding this comment.
Consider making these retry parameters configurable through the Config struct rather than hardcoding them.
| @@ -147,7 +163,7 @@ | |||
| } | |||
| } | |||
| _ => { | |||
| tracing::info!("Unknown model name: {}", model.name); | |||
| tracing::warn!("Unknown model name: {}", model.name); | |||
| continue; | |||
| } | |||
| }; | |||
There was a problem hiding this comment.
Consider adding error handling here instead of using unwrap(). A failure to send could indicate a channel closure which should be handled gracefully.
| discord_message_sender.run().await; | ||
| tracing::info!("Starting Torii client subscriber"); | ||
| let torii_client_subscriber = | ||
| ToriiClientSubscriber::new(config_clone, processed_event_sender) |
There was a problem hiding this comment.
Consider adding a timeout or cancellation mechanism for these spawned tasks to ensure clean shutdown.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (4)
discord-bot/src/actors/torii_client_subscriber.rs (2)
62-99: Consider enhancing error handling and logging.While the current implementation is good, consider these improvements:
- Log the attempt number during retries
- Add more context to error messages
- Consider implementing a circuit breaker pattern for better resilience
while tries < max_num_tries { + tracing::info!("Attempting to connect (attempt {}/{})", tries + 1, max_num_tries); let rcv = self .client .on_event_message_updated( vec![EntityKeysClause::Keys(KeysClause { keys: vec![], pattern_matching: PatternMatching::VariableLen, models: TORII_SUBSCRIPTION_MODELS .iter() .map(|s| s.to_string()) .collect(), })], false, ) .await; match rcv { Ok(mut rcv) => { + tracing::info!("Successfully connected to Torii"); backoff = Duration::from_secs(1); loop { match rcv.next().await { Some(result) => { if let Ok((_, entity)) = result { tracing::info!("Received event"); self.treat_received_torii_event(entity).await; } else { tracing::warn!( - "Received invalid data from torii, reconnecting" + "Received invalid data from torii, reconnecting after attempt {}", + tries + 1 ); break; } } None => { - tracing::warn!("Stream returned an error, reconnecting"); + tracing::warn!("Stream ended unexpectedly, reconnecting after attempt {}", + tries + 1 + ); break; } } } } Err(e) => { - tracing::warn!("Subscription failed: {:?}", e); + tracing::warn!( + "Subscription failed on attempt {}/{}: {:?}", + tries + 1, + max_num_tries, + e + ); tries += 1; } }
102-112: Consider adding graceful shutdown mechanism.The current implementation has a good retry mechanism, but it could benefit from:
- A graceful shutdown mechanism (e.g., handling SIGTERM)
- More actionable error messages
- Metrics for monitoring reconnection attempts
Consider implementing a shutdown channel:
pub async fn subscribe(mut self, mut shutdown: tokio::sync::broadcast::Receiver<()>) { tracing::info!("Setting up Torii client"); let mut tries = 0; let max_num_tries = 200; let mut backoff = Duration::from_secs(1); let max_backoff = Duration::from_secs(60); while tries < max_num_tries { tokio::select! { _ = shutdown.recv() => { tracing::info!("Received shutdown signal, stopping Torii client"); return; } _ = async { // ... existing subscription logic ... } => {} } } tracing::error!("Torii client disconnected after {} attempts. Please check network connectivity and Torii service status", max_num_tries); }discord-bot/src/init.rs (2)
55-56: Avoid unnecessary cloning ofdiscord_token.In line 55,
config_clone.discord_token.clone()clones thediscord_token. Ifdiscord_tokenimplementsAsRef<str>, you can avoid cloning by passing a reference.Apply this diff to optimize:
Arc::new(Http::new(&config_clone.discord_token.clone())), +Arc::new(Http::new(&config_clone.discord_token)),
84-87: Simplify error handling when building the Discord client.Using
.map_err(shuttle_runtime::CustomError::new)?wraps the error but might not provide additional context. If not necessary, you can use the?operator directly to propagate the error.Consider this change:
.await - .map_err(shuttle_runtime::CustomError::new)?; + ?;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
discord-bot/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
discord-bot/Cargo.toml(1 hunks)discord-bot/src/actors/discord_message_sender.rs(0 hunks)discord-bot/src/actors/torii_client_subscriber.rs(4 hunks)discord-bot/src/init.rs(4 hunks)discord-bot/src/main.rs(2 hunks)
💤 Files with no reviewable changes (1)
- discord-bot/src/actors/discord_message_sender.rs
🔇 Additional comments (5)
discord-bot/src/main.rs (1)
13-13: LGTM: Import change aligns with service initialization refactoring
discord-bot/Cargo.toml (1)
35-39: Verify shuttle upgrade compatibility with reconnection feature
The upgrade of shuttle dependencies from 0.48.0 to 0.49.0 might be related to improved reconnection handling. Let's verify the changelog for relevant improvements.
Consider documenting the specific shuttle features or improvements that this version upgrade provides for the reconnection functionality. This will help with future maintenance and upgrades.
discord-bot/src/actors/torii_client_subscriber.rs (2)
21-28: LGTM! Well-structured constant definition.
Good practice to centralize the model names in a constant. This improves maintainability and reduces the risk of typos.
166-166: Verify model handling completeness.
The warning for unknown models is appropriate. Let's verify that all expected models are handled.
✅ Verification successful
Let me gather more information to see the complete match statement since the current output is truncated.
Let me get one final piece of information to see the complete match statement since it's still truncated.
Let me get one final check with even more lines to see the complete SettleRealmData case.
Model handling is complete and correct
All 6 models defined in TORII_SUBSCRIPTION_MODELS are properly handled in the match statement:
- eternum-BattleStartData
- eternum-BattleJoinData
- eternum-BattleLeaveData
- eternum-BattleClaimData
- eternum-BattlePillageData
- eternum-SettleRealmData
The warning for unknown models serves as a proper fallback case. The logging level change from info to warn is appropriate.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all models defined in TORII_SUBSCRIPTION_MODELS are handled in the match statement
# Search for model definitions and match arms
echo "Defined models:"
rg "const TORII_SUBSCRIPTION_MODELS.*=.*\[" -A 10 discord-bot/src/actors/torii_client_subscriber.rs
echo "\nHandled models in match statement:"
rg "match model\.name\.as_str\(\) \{" -A 20 discord-bot/src/actors/torii_client_subscriber.rs
Length of output: 1561
Script:
#!/bin/bash
# Get the complete match statement with more context lines
rg "match model\.name\.as_str\(\) \{" -A 30 discord-bot/src/actors/torii_client_subscriber.rs
Length of output: 1478
Script:
#!/bin/bash
# Get the complete match statement with even more context lines
rg "match model\.name\.as_str\(\) \{" -A 40 discord-bot/src/actors/torii_client_subscriber.rs
Length of output: 1928
Script:
#!/bin/bash
# Get the complete match statement with maximum context lines
rg "match model\.name\.as_str\(\) \{" -A 50 discord-bot/src/actors/torii_client_subscriber.rs
Length of output: 2263
discord-bot/src/init.rs (1)
89-89: Verify the conversion to ShuttleSerenity.
Ensure that converting the Client into ShuttleSerenity using client.into() correctly transfers all necessary states and that no important information is lost in the conversion.
| let client = launch_services(config, pool) | ||
| .await | ||
| .expect("Failed to setup discord services"); | ||
|
|
||
| Ok(client.into()) | ||
| Ok(client) |
There was a problem hiding this comment.
💡 Codebase verification
🛠️ Refactor suggestion
Error handling inconsistency confirmed - needs standardization
The codebase shows mixed error handling patterns:
- Uses
.context()for migrations - Uses
.expect()in multiple places including config loading and service initialization
Consider standardizing error handling across the codebase:
discord-bot/src/main.rs: Both config loading and service launch use.expect()discord-bot/src/init.rs: Contains.expect()in Torii client initialization
Recommended approach:
// For main.rs service launch
let client = launch_services(config, pool)
.await
.context("failed to setup discord services")?;🔗 Analysis chain
Improve error handling consistency and robustness
The current error handling approach has two issues:
- Using
expectin production code could lead to abrupt termination - Inconsistent error handling style (
.context()vs.expect())
Consider applying this more robust error handling approach:
- let client = launch_services(config, pool)
- .await
- .expect("Failed to setup discord services");
-
- Ok(client)
+ let client = launch_services(config, pool)
+ .await
+ .context("failed to setup discord services")?;
+
+ Ok(client)Let's verify the integration with the new service launch mechanism:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the service launch implementation and its usage
# Expected: Find the launch_services implementation and any other direct usages
# Check the implementation of launch_services
ast-grep --pattern 'async fn launch_services($$$) {
$$$
}'
# Check for any other usages of launch_services
rg -l 'launch_services'
Length of output: 134
Script:
#!/bin/bash
# Let's check the implementation details and error handling patterns
# Check launch_services implementation
rg -A 10 "launch_services" discord-bot/src/init.rs
# Check error handling patterns in the codebase
rg "\.context|\.expect" discord-bot/src/
Length of output: 891
| dojo-types = { git = "https://github.com/edisontim/dojo", rev = "756027b" } | ||
| torii-client = { git = "https://github.com/edisontim/dojo", rev = "756027b" } | ||
| torii-grpc = { git = "https://github.com/edisontim/dojo", features = [ | ||
| "client", | ||
| ], tag = "v1.0.0-rc.1" } | ||
| torii-relay = { git = "https://github.com/dojoengine/dojo", tag = "v1.0.0-rc.1" } | ||
| dojo-world = { git = "https://github.com/dojoengine/dojo", tag = "v1.0.0-rc.1" } | ||
| ], rev = "756027b" } | ||
| torii-relay = { git = "https://github.com/edisontim/dojo", rev = "756027b" } | ||
| dojo-world = { git = "https://github.com/edisontim/dojo", rev = "756027b" } |
There was a problem hiding this comment.
💡 Codebase verification
Using a personal fork raises stability and maintenance concerns
The verification shows that while edisontim/dojo is an active fork (last updated 2024-11-07), using it instead of the official repository introduces several risks:
- The fork is actively diverging from the main repository with recent commits unrelated to the reconnection feature
- The specific commit
756027bcontaining keepalive changes is not visible in recent history - There's no clear integration path back to the official repository
Recommendations:
- Consider using the official
dojoengine/dojorepository instead of a personal fork - If the keepalive changes are necessary, they should be proposed upstream via a pull request
- Document the rationale for using this specific fork and commit if it must be maintained
🔗 Analysis chain
Verify the stability and maintenance of the forked dojo dependencies
The switch from the official dojoengine/dojo repository to a personal fork edisontim/dojo at a specific commit raises some concerns:
- Long-term maintenance and updates from the upstream repository
- Security implications of using a forked version
- Relationship between the fork and the reconnection feature
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the fork's commit and its relationship to the official repository
# Check if the commit exists in the official repository
gh api repos/dojoengine/dojo/commits/756027b --silent || echo "Commit not found in official repository"
# Get commit details from the fork
gh api repos/edisontim/dojo/commits/756027b --jq '.commit.message'
# Check for any open PRs from this fork to the main repository
gh api search/issues --jq '.items[] | {title:.title, url:.html_url}' \
-f q='is:pr repo:dojoengine/dojo author:edisontim'
Length of output: 523
Script:
#!/bin/bash
# Let's try a different approach to verify the fork and its relationship
# Check if the fork exists and when it was last updated
gh api repos/edisontim/dojo --jq '.updated_at, .fork'
# List recent commits in the fork to understand activity
gh api repos/edisontim/dojo/commits --jq '.[0:3] | .[] | {sha: .sha, message: .commit.message, date: .commit.author.date}'
# Check if there are any references to reconnection or keepalive in the commit history
gh api repos/edisontim/dojo/commits --jq '.[] | select(.commit.message | contains("keepalive") or contains("reconnect")) | {sha: .sha, message: .commit.message}'
Length of output: 1020
| pub async fn launch_services(config: Config, pool: PgPool) -> ShuttleSerenity { | ||
| launch_internal_services(pool.clone(), config.clone()).await; | ||
|
|
||
| launch_discord_service(config, pool).await | ||
| } |
There was a problem hiding this comment.
Ensure error handling for launch_internal_services.
In launch_services, you call launch_internal_services and await it, but since launch_internal_services does not return a Result, any errors within it might not be properly handled or propagated. Consider modifying launch_internal_services to return a Result or ensure all errors are handled within the function.
| tracing::info!("Starting message dispatcher"); | ||
| let mut discord_message_sender = DiscordMessageSender::new( | ||
| Arc::new(Http::new(&config_clone.discord_token.clone())), | ||
| message_receiver, | ||
| ); | ||
| discord_message_sender.run().await; |
There was a problem hiding this comment.
Handle potential errors in DiscordMessageSender.
Similarly, in the tokio::spawn block for discord_message_sender.run().await, any errors are not captured or logged. This could mask issues occurring within the message sender.
Apply this diff to handle errors:
discord_message_sender.run().await;
+ if let Err(e) = discord_message_sender.run().await {
+ tracing::error!("Error in DiscordMessageSender: {:?}", e);
+ }Committable suggestion skipped: line range outside the PR's diff.
| tracing::info!("Starting Torii client subscriber"); | ||
| let torii_client_subscriber = | ||
| ToriiClientSubscriber::new(config_clone, processed_event_sender) | ||
| .await | ||
| .expect("Failed to create Torii client subscriber"); | ||
| torii_client_subscriber.subscribe().await; |
There was a problem hiding this comment.
Add error handling in the spawned task for ToriiClientSubscriber.
Within the tokio::spawn block, if torii_client_subscriber.subscribe().await results in an error, it is not currently being handled or logged. This could lead to silent failures.
Apply this diff to handle errors:
torii_client_subscriber.subscribe().await;
+ if let Err(e) = torii_client_subscriber.subscribe().await {
+ tracing::error!("Error in ToriiClientSubscriber: {:?}", e);
+ }Committable suggestion skipped: line range outside the PR's diff.
Closes #2008
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Documentation