[MM-24753] Improves error message when plugin crashes during slash command (#15334)

Co-authored-by: Ali Farooq <ali.farooq0@pm.me>
Co-authored-by: Ben Schumacher <ben.schumacher@mattermost.com>

app/plugin_commands.go

@@ -4,6 +4,7 @@
 package app
 
 import (
+	"fmt"
 	"net/http"
 	"net/url"
 	"strings"
@@ -136,6 +137,11 @@ func (a *App) tryExecutePluginCommand(args *model.CommandArgs) (*model.Command,
 		return nil, nil, nil
 	}
 
+	// Checking if plugin is working or not
+	if err := pluginsEnvironment.PerformHealthCheck(matched.PluginId); err != nil {
+		return matched.Command, nil, model.NewAppError("ExecutePluginCommand", "model.plugin_command_error.error.app_error", map[string]interface{}{"Command": trigger}, "err= Plugin has recently crashed: "+matched.PluginId, http.StatusInternalServerError)
+	}
+
 	pluginHooks, err := pluginsEnvironment.HooksForPlugin(matched.PluginId)
 	if err != nil {
 		return matched.Command, nil, model.NewAppError("ExecutePluginCommand", "model.plugin_command.error.app_error", nil, "err="+err.Error(), http.StatusInternalServerError)
@@ -150,5 +156,11 @@ func (a *App) tryExecutePluginCommand(args *model.CommandArgs) (*model.Command,
 	}
 
 	response, appErr := pluginHooks.ExecuteCommand(a.PluginContext(), args)
+
+	// Checking if plugin crashed after running the command
+	if err := pluginsEnvironment.PerformHealthCheck(matched.PluginId); err != nil {
+		errMessage := fmt.Sprintf("err= Plugin %s crashed due to /%s command", matched.PluginId, trigger)
+		return matched.Command, nil, model.NewAppError("ExecutePluginCommand", "model.plugin_command_crash.error.app_error", map[string]interface{}{"Command": trigger, "PluginId": matched.PluginId}, errMessage, http.StatusInternalServerError)
+	}
 	return matched.Command, response, appErr
 }

app/plugin_commands_test.go

@@ -286,4 +286,94 @@ func TestPluginCommand(t *testing.T) {
 
 		th.App.RemovePlugin(pluginIds[0])
 	})
+	t.Run("plugin has crashed before execution of command", func(t *testing.T) {
+		tearDown, pluginIds, activationErrors := SetAppEnvironmentWithPlugins(t, []string{`
+			package main
+
+			import (
+				"github.com/mattermost/mattermost-server/v5/plugin"
+				"github.com/mattermost/mattermost-server/v5/model"
+			)
+
+			type MyPlugin struct {
+				plugin.MattermostPlugin
+
+			}
+
+			func (p *MyPlugin) OnActivate() error {
+				err := p.API.RegisterCommand(&model.Command{
+					Trigger: "code",
+				})
+				if err != nil {
+					p.API.LogError("error", "err", err)
+				}
+				panic("Uncaught Error")
+
+				return err
+			}
+
+			func (p *MyPlugin) ExecuteCommand(c *plugin.Context, args *model.CommandArgs) (*model.CommandResponse, *model.AppError) {
+				return &model.CommandResponse{}, nil
+			}
+
+			func main() {
+				plugin.ClientMain(&MyPlugin{})
+			}
+		`}, th.App, th.App.NewPluginAPI)
+		defer tearDown()
+		require.Len(t, activationErrors, 1)
+		require.Nil(t, nil, activationErrors[0])
+		args.Command = "/code"
+		resp, err := th.App.ExecuteCommand(args)
+		require.Nil(t, resp)
+		require.NotNil(t, err)
+		require.Equal(t, err.Id, "model.plugin_command_error.error.app_error")
+		th.App.RemovePlugin(pluginIds[0])
+	})
+
+	t.Run("plugin has crashed due to the execution of the command", func(t *testing.T) {
+		tearDown, pluginIds, activationErrors := SetAppEnvironmentWithPlugins(t, []string{`
+			package main
+
+			import (
+				"github.com/mattermost/mattermost-server/v5/plugin"
+				"github.com/mattermost/mattermost-server/v5/model"
+			)
+
+			type MyPlugin struct {
+				plugin.MattermostPlugin
+
+			}
+
+			func (p *MyPlugin) OnActivate() error {
+				err := p.API.RegisterCommand(&model.Command{
+					Trigger: "code",
+				})
+				if err != nil {
+					p.API.LogError("error", "err", err)
+				}
+
+				return err
+			}
+
+			func (p *MyPlugin) ExecuteCommand(c *plugin.Context, args *model.CommandArgs) (*model.CommandResponse, *model.AppError) {
+				panic("Uncaught Error")
+				return &model.CommandResponse{}, nil
+			}
+
+			func main() {
+				plugin.ClientMain(&MyPlugin{})
+			}
+		`}, th.App, th.App.NewPluginAPI)
+		defer tearDown()
+		require.Len(t, activationErrors, 1)
+		require.Nil(t, nil, activationErrors[0])
+		args.Command = "/code"
+		resp, err := th.App.ExecuteCommand(args)
+		require.Nil(t, resp)
+		require.NotNil(t, err)
+		require.Equal(t, err.Id, "model.plugin_command_crash.error.app_error")
+		th.App.RemovePlugin(pluginIds[0])
+	})
+
 }

i18n/en.json

@@ -7302,6 +7302,14 @@
     "id": "model.plugin_command.error.app_error",
     "translation": "An error occurred while trying to execute this command."
   },
+  {
+    "id": "model.plugin_command_crash.error.app_error",
+    "translation": "/{{.Command}} command crashed the {{.PluginId}} plugin. Please contact your system administrator"
+  },
+  {
+    "id": "model.plugin_command_error.error.app_error",
+    "translation": "Plugin for /{{.Command}} is not working. Please contact your system administrator"
+  },
   {
     "id": "model.plugin_key_value.is_valid.key.app_error",
     "translation": "Invalid key, must be more than {{.Min}} and a of maximum {{.Max}} characters long."

plugin/environment.go

@@ -472,8 +472,8 @@ func (env *Environment) RunMultiPluginHook(hookRunnerFunc func(hooks Hooks) bool
 	}
 }
 
-// performHealthCheck uses the active plugin's supervisor to verify if the plugin has crashed.
-func (env *Environment) performHealthCheck(id string) error {
+// PerformHealthCheck uses the active plugin's supervisor to verify if the plugin has crashed.
+func (env *Environment) PerformHealthCheck(id string) error {
 	p, ok := env.registeredPlugins.Load(id)
 	if !ok {
 		return nil

plugin/health_check.go

@@ -51,7 +51,7 @@ func (job *PluginHealthCheckJob) run() {
 // If the plugin passes the health check, do nothing.
 // If the plugin fails the health check, the function either restarts or deactivates the plugin, based on the quantity and frequency of its failures.
 func (job *PluginHealthCheckJob) CheckPlugin(id string) {
-	err := job.env.performHealthCheck(id)
+	err := job.env.PerformHealthCheck(id)
 	if err == nil {
 		return
 	}