From 53ae0e3d7066c7364aa21b65b3041abf7043ae22 Mon Sep 17 00:00:00 2001
From: Ske <voltasalt@gmail.com>
Date: Tue, 22 Oct 2019 19:31:40 +0200
Subject: [PATCH] Improve error handling and reporting after command rewrite

---
 PluralKit.Bot/Bot.cs                   | 36 +++++++++++++++++++++++---
 PluralKit.Bot/CommandSystem/Context.cs |  5 ++++
 PluralKit.Bot/Utils.cs                 | 20 ++++++++++++++
 3 files changed, 57 insertions(+), 4 deletions(-)

diff --git a/PluralKit.Bot/Bot.cs b/PluralKit.Bot/Bot.cs
index 8951ed41..b2aa147b 100644
--- a/PluralKit.Bot/Bot.cs
+++ b/PluralKit.Bot/Bot.cs
@@ -85,7 +85,8 @@ namespace PluralKit.Bot
             .AddSingleton<IDiscordClient, DiscordShardedClient>(_ => new DiscordShardedClient(new DiscordSocketConfig
             {
                 MessageCacheSize = 5,
-                ExclusiveBulkDelete = true
+                ExclusiveBulkDelete = true,
+                DefaultRetryMode = RetryMode.AlwaysRetry
             }))
             .AddSingleton<Bot>()
             .AddTransient<CommandTree>()
@@ -296,9 +297,10 @@ namespace PluralKit.Bot
             logger.Error(e, "Exception in bot event handler");
             
             var evt = new SentryEvent(e);
-            SentrySdk.CaptureEvent(evt, scope);
             
-            Console.Error.WriteLine(e);
+            // Don't blow out our Sentry budget on sporadic not-our-problem erorrs
+            if (e.IsOurProblem())
+                SentrySdk.CaptureEvent(evt, scope);
         }
     }
     
@@ -359,7 +361,17 @@ namespace PluralKit.Bot
                         "select systems.* from systems, accounts where accounts.uid = @Id and systems.id = accounts.system",
                         new {Id = msg.Author.Id});
 
-                await _tree.ExecuteCommand(new Context(_services, msg, argPos, system));
+                try
+                {
+                    await _tree.ExecuteCommand(new Context(_services, msg, argPos, system));
+                }
+                catch (Exception e)
+                {
+                    await HandleCommandError(msg, e);
+                    // HandleCommandError only *reports* the error, we gotta pass it through to the parent
+                    // error handler by rethrowing:
+                    throw;
+                }
             }
             else
             {
@@ -375,6 +387,22 @@ namespace PluralKit.Bot
             }
         }
 
+        private async Task HandleCommandError(SocketUserMessage msg, Exception exception)
+        {
+            // This function *specifically* handles reporting a command execution error to the user.
+            // We'll fetch the event ID and send a user-facing error message.
+            // ONLY IF this error's actually our problem. As for what defines an error as "our problem",
+            // check the extension method :)
+            if (exception.IsOurProblem())
+            {
+                var eid = _services.GetService<EventIdProvider>().EventId;
+                await msg.Channel.SendMessageAsync(
+                    $"{Emojis.Error} Internal error occurred. Please join the support server (https://discord.gg/PczBt78), and send the developer this ID: `{eid}`");
+            }
+            
+            // If not, don't care. lol.
+        }
+
         private void RegisterMessageMetrics(SocketMessage msg)
         {
             _metrics.Measure.Meter.Mark(BotMetrics.MessagesReceived);
diff --git a/PluralKit.Bot/CommandSystem/Context.cs b/PluralKit.Bot/CommandSystem/Context.cs
index 99854d41..7732d6b2 100644
--- a/PluralKit.Bot/CommandSystem/Context.cs
+++ b/PluralKit.Bot/CommandSystem/Context.cs
@@ -86,6 +86,11 @@ namespace PluralKit.Bot.CommandSystem
             {
                 await Reply($"{Emojis.Error} {e.Message}");
             }
+            catch (TimeoutException e)
+            {
+                // Got a complaint the old error was a bit too patronizing. Hopefully this is better?
+                await Reply($"{Emojis.Error} Operation timed out, sorry. Try again, perhaps?");
+            }
         }
 
         public async Task<IUser> MatchUser()
diff --git a/PluralKit.Bot/Utils.cs b/PluralKit.Bot/Utils.cs
index 3bac36fa..91cf5b93 100644
--- a/PluralKit.Bot/Utils.cs
+++ b/PluralKit.Bot/Utils.cs
@@ -2,9 +2,11 @@ using System;
 using System.Globalization;
 using System.Linq;
 using System.Net.Http;
+using System.Net.Sockets;
 using System.Text.RegularExpressions;
 using System.Threading.Tasks;
 using Discord;
+using Discord.Net;
 
 using PluralKit.Core;
 using Image = SixLabors.ImageSharp.Image;
@@ -117,6 +119,24 @@ namespace PluralKit.Bot
 
         public static async Task<bool> HasPermission(this IChannel channel, ChannelPermission permission) =>
             (await PermissionsIn(channel)).Has(permission);
+
+        public static bool IsOurProblem(this Exception e)
+        {
+            // This function filters out sporadic errors out of our control from being reported to Sentry
+            // otherwise we'd blow out our error reporting budget as soon as Discord takes a dump, or something.
+            
+            // Discord server errors are *not our problem*
+            if (e is HttpException he && ((int) he.HttpCode) >= 500) return false;
+            
+            // Socket errors are *not our problem*
+            if (e is SocketException) return false;
+            
+            // Tasks being cancelled for whatver reason are, you guessed it, also not our problem.
+            if (e is TaskCanceledException) return false;
+            
+            // This may expanded at some point.
+            return true;
+        }
     }
 
     /// <summary>