From 05cdb368862c4ea35f438a10320897ef63c5aadc Mon Sep 17 00:00:00 2001
From: Agniva De Sarker <agnivade@yahoo.co.in>
Date: Tue, 21 Jan 2025 11:15:19 +0530
Subject: [PATCH] MM-49353: Setup intermediate signal handlers for plugin
 shutdown (#28653)

There are several steps that a server runs through inside (*Server).Start
after ch.initPlugins() till the signal handler is reached which handles
the server shutdown procedure.

The issue arises when the server is shutdown after ch.initPlugins() completes
but before (*Server).Start finishes. In that case, the plugins are all started
but they won't be shut down cleanly.

To fix this edge-case, we set up an intermediate signal handler, which
attaches itself as soon as ch.initPlugins is finished, allowing us to run
the cleanup code in case the shutdown happens before (*Server).Start finishes.

And when we do reach the main signal handler, we don't need this intermediate
handler any more. So we just reset the handlers and use the main signal handler
which takes care of shutting down the whole server.

Note: This is still not 100% bug-proof because ch.initPlugins() will initialize
_all_ plugins, and the shutdown can happen just after one plugin is initialized.
To handle that case will require the need to set up signal handlers after every
plugin init which feels like overkill to me.

A sample flow diagram to visualize better:

Edge-case
server.Start()
|
ch.initPlugins()
|
<ctrl-c>
|
execute signal handler, os.Exit(1)

Happy-path
server.Start()
|
ch.initPlugins()
|
server.Start() finished
|
reset old signal handler
|
setup main signal handler
|
server runs on as usual until shutdown

https://mattermost.atlassian.net/browse/MM-49353

```release-note
NONE
```
---
 server/channels/app/channels.go          | 33 +++++++++++++++++++-----
 server/cmd/mattermost/commands/server.go |  4 +++
 server/public/plugin/environment.go      |  2 +-
 3 files changed, 32 insertions(+), 7 deletions(-)

diff --git a/server/channels/app/channels.go b/server/channels/app/channels.go
index b6ed4be31e..cc10f3643b 100644
--- a/server/channels/app/channels.go
+++ b/server/channels/app/channels.go
@@ -4,9 +4,12 @@
 package app
 
 import (
+	"os"
+	"os/signal"
 	"runtime"
 	"strings"
 	"sync"
+	"syscall"
 
 	"github.com/pkg/errors"
 
@@ -77,6 +80,7 @@ type Channels struct {
 	postReminderMut  sync.Mutex
 	postReminderTask *model.ScheduledTask
 
+	interruptQuitChan chan struct{}
 	scheduledPostMut  sync.Mutex
 	scheduledPostTask *model.ScheduledTask
 	loginAttemptsMut  sync.Mutex
@@ -84,12 +88,13 @@ type Channels struct {
 
 func NewChannels(s *Server) (*Channels, error) {
 	ch := &Channels{
-		srv:             s,
-		imageProxy:      imageproxy.MakeImageProxy(s.platform, s.httpService, s.Log()),
-		uploadLockMap:   map[string]bool{},
-		filestore:       s.FileBackend(),
-		exportFilestore: s.ExportFileBackend(),
-		cfgSvc:          s.Platform(),
+		srv:               s,
+		imageProxy:        imageproxy.MakeImageProxy(s.platform, s.httpService, s.Log()),
+		uploadLockMap:     map[string]bool{},
+		filestore:         s.FileBackend(),
+		exportFilestore:   s.ExportFileBackend(),
+		cfgSvc:            s.Platform(),
+		interruptQuitChan: make(chan struct{}),
 	}
 
 	// We are passing a partially filled Channels struct so that the enterprise
@@ -159,6 +164,20 @@ func (ch *Channels) Start() error {
 	ctx := request.EmptyContext(ch.srv.Log())
 	ch.initPlugins(ctx, *ch.cfgSvc.Config().PluginSettings.Directory, *ch.cfgSvc.Config().PluginSettings.ClientDirectory)
 
+	interruptChan := make(chan os.Signal, 1)
+	signal.Notify(interruptChan, syscall.SIGINT, syscall.SIGTERM)
+	go func() {
+		select {
+		case <-interruptChan:
+			if err := ch.Stop(); err != nil {
+				ch.srv.Log().Warn("Error stopping channels", mlog.Err(err))
+			}
+			os.Exit(1)
+		case <-ch.interruptQuitChan:
+			return
+		}
+	}()
+
 	ch.AddConfigListener(func(prevCfg, cfg *model.Config) {
 		// We compute the difference between configs
 		// to ensure we don't re-init plugins unnecessarily.
@@ -208,6 +227,8 @@ func (ch *Channels) Stop() error {
 	}
 	ch.dndTaskMut.Unlock()
 
+	close(ch.interruptQuitChan)
+
 	return nil
 }
 
diff --git a/server/cmd/mattermost/commands/server.go b/server/cmd/mattermost/commands/server.go
index 642bd95b78..76d33bfaf3 100644
--- a/server/cmd/mattermost/commands/server.go
+++ b/server/cmd/mattermost/commands/server.go
@@ -109,6 +109,10 @@ func runServer(configStore *config.Store, interruptChan chan os.Signal) error {
 
 	notifyReady()
 
+	// Wiping off any signal handlers set before.
+	// This may come from intermediary signal handlers requiring to clean
+	// up resources before server.Start can finish.
+	signal.Reset(syscall.SIGINT, syscall.SIGTERM)
 	// wait for kill signal before attempting to gracefully shutdown
 	// the running service
 	signal.Notify(interruptChan, syscall.SIGINT, syscall.SIGTERM)
diff --git a/server/public/plugin/environment.go b/server/public/plugin/environment.go
index 289b55ea83..b8f46e60f0 100644
--- a/server/public/plugin/environment.go
+++ b/server/public/plugin/environment.go
@@ -483,7 +483,7 @@ func (env *Environment) Shutdown() {
 	env.TogglePluginHealthCheckJob(false)
 
 	var wg sync.WaitGroup
-	env.registeredPlugins.Range(func(key, value any) bool {
+	env.registeredPlugins.Range(func(_, value any) bool {
 		rp := value.(registeredPlugin)
 
 		if rp.supervisor == nil || !env.IsActive(rp.BundleInfo.Manifest.Id) {