Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: discord bot reconnection #2105

Merged
merged 8 commits into from
Nov 15, 2024
Merged

Conversation

edisontim
Copy link
Collaborator

@edisontim edisontim commented Nov 14, 2024

Closes #2008

Summary by CodeRabbit

  • New Features

    • Introduced new methods for improved service initialization and error handling.
    • Added a constant for battle event models to enhance subscription clarity.
  • Bug Fixes

    • Enhanced error logging for subscription failures and invalid data.
  • Refactor

    • Restructured service initialization for clearer separation of concerns.
    • Updated dependency sources and versions for improved functionality.
  • Documentation

    • Improved clarity in error handling and logging messages throughout the code.

Copy link

vercel bot commented Nov 14, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
eternum ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 14, 2024 5:08pm
1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
eternum-landing ⬜️ Ignored (Inspect) Visit Preview Nov 14, 2024 5:08pm

Copy link
Contributor

coderabbitai bot commented Nov 14, 2024

Walkthrough

The pull request includes significant updates to the Cargo.toml file for the eternum-discord package, focusing on dependency management. Key changes involve updating the source repositories for several dojo dependencies and incrementing the versions of multiple shuttle dependencies. Additionally, modifications were made to several Rust source files, including changes to logging in the DiscordMessageSender, enhancements to error handling in the ToriiClientSubscriber, and a restructuring of service initialization in the init.rs and main.rs files.

Changes

File Path Change Summary
discord-bot/Cargo.toml Updated dependencies for dojo-types, torii-client, torii-grpc, torii-relay, and dojo-world to a new source repository. Incremented versions of shuttle-runtime, shuttle-serenity, shuttle-rocket, and shuttle-shared-db from 0.48.0 to 0.49.0.
discord-bot/src/actors/discord_message_sender.rs Removed logging statements in run and send_message methods.
discord-bot/src/actors/torii_client_subscriber.rs Added constant TORII_SUBSCRIPTION_MODELS. Changed subscribe method to use a while loop for retries and improved error handling and logging.
discord-bot/src/init.rs Renamed init_inner_services to launch_services, which now returns ShuttleSerenity. Introduced launch_internal_services and launch_discord_service for better service initialization.
discord-bot/src/main.rs Replaced init_services with launch_services in the main function, updated client handling, and enhanced error handling for migration execution.

Possibly related PRs

  • feat: make messages clearer + refactor discord bot #2015: The changes in the main PR regarding the update of dependencies related to the dojo project are relevant to this PR, as both involve modifications to the Cargo.toml file, although the specific dependencies and their versions differ.

Suggested reviewers

  • ponderingdemocritus

Poem

🐇 In the garden of code, we hop and play,
Dependencies shift, in a brand new way.
With services launching, and logs trimmed bright,
Our Discord bot dances, in the soft moonlight.
Hooray for the changes, let’s cheer and rejoice,
For every new feature, we’ll raise our voice! 🎉


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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

Failed to generate code suggestions for PR

Copy link
Contributor

@mentatbot mentatbot bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Consider making retry parameters configurable
  2. Add proper error handling for channel operations
  3. Consider adding shutdown mechanisms for spawned tasks
  4. 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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
}
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider adding a timeout or cancellation mechanism for these spawned tasks to ensure clean shutdown.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Log the attempt number during retries
  2. Add more context to error messages
  3. 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:

  1. A graceful shutdown mechanism (e.g., handling SIGTERM)
  2. More actionable error messages
  3. 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 of discord_token.

In line 55, config_clone.discord_token.clone() clones the discord_token. If discord_token implements AsRef<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

📥 Commits

Reviewing files that changed from the base of the PR and between 5b1e27b and c7231d8.

⛔ Files ignored due to path filters (1)
  • discord-bot/Cargo.lock is 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.

Comment on lines +42 to +46
let client = launch_services(config, pool)
.await
.expect("Failed to setup discord services");

Ok(client.into())
Ok(client)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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:

  1. Using expect in production code could lead to abrupt termination
  2. 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

Comment on lines +23 to +29
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" }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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:

  1. The fork is actively diverging from the main repository with recent commits unrelated to the reconnection feature
  2. The specific commit 756027b containing keepalive changes is not visible in recent history
  3. There's no clear integration path back to the official repository

Recommendations:

  • Consider using the official dojoengine/dojo repository 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:

  1. Long-term maintenance and updates from the upstream repository
  2. Security implications of using a forked version
  3. 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

Comment on lines +19 to +23
pub async fn launch_services(config: Config, pool: PgPool) -> ShuttleSerenity {
launch_internal_services(pool.clone(), config.clone()).await;

launch_discord_service(config, pool).await
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Comment on lines +53 to +58
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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Comment on lines +31 to +36
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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

@ponderingdemocritus ponderingdemocritus merged commit e8f3cb6 into next Nov 15, 2024
7 checks passed
@ponderingdemocritus ponderingdemocritus deleted the feat/discord-bot-reconnection branch November 15, 2024 19:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[discord-bot] Fix
3 participants