From 5875acf7d9cee79e0a383975d53fa79a6573ac8d Mon Sep 17 00:00:00 2001
From: chrislu <chris.lu@gmail.com>
Date: Thu, 20 Nov 2025 19:42:55 -0800
Subject: [PATCH] improve: call IAM server Shutdown() for best-effort cleanup
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Added call to iamApiServer.Shutdown() to ensure cleanup happens when possible,
and documented the limitations of the current approach.

Problem:
  The Shutdown() method was defined in IamApiServer but never called anywhere,
  meaning the KeepConnectedToMaster goroutine would continue running even when
  the IAM server stopped, causing resource leaks.

Changes:
  1. Store iamApiServer instance in weed/command/iam.go
     - Changed: _, iamApiServer_err := iamapi.NewIamApiServer(...)
     - To: iamApiServer, iamApiServer_err := iamapi.NewIamApiServer(...)

  2. Added defer call for best-effort cleanup
     - defer iamApiServer.Shutdown()
     - This will execute if startIamServer() returns normally

  3. Added logging in Shutdown() method
     - Log when shutdown is triggered for visibility

  4. Documented limitations and future improvements
     - Added note that defer only works for normal function returns
     - SeaweedFS commands don't currently have signal handling
     - Suggested future enhancement: add SIGTERM/SIGINT handling

Current behavior:
  - ✓ Cleanup happens if HTTP server fails to start (glog.Fatalf path)
  - ✓ Cleanup happens if Serve() returns with error (unlikely)
  - ✗ Cleanup does NOT happen on SIGTERM/SIGINT (process killed)

The last case is a limitation of the current command architecture - all
SeaweedFS commands (s3, filer, volume, master, iam) lack signal handling
for graceful shutdown. This is a systemic issue that affects all services.

Future enhancement:
  To properly handle SIGTERM/SIGINT, the command layer would need:

    sigChan := make(chan os.Signal, 1)
    signal.Notify(sigChan, syscall.SIGTERM, syscall.SIGINT)

    go func() {
      httpServer.Serve(listener) // Non-blocking
    }()

    <-sigChan
    glog.V(0).Infof("Received shutdown signal")
    iamApiServer.Shutdown()
    httpServer.Shutdown(context.Background())

This would require refactoring the command structure for all services,
which is out of scope for this change.

Benefits of current approach:
  ✓ Best-effort cleanup (better than nothing)
  ✓ Proper cleanup in error paths
  ✓ Documented for future improvement
  ✓ Consistent with how other SeaweedFS services handle lifecycle
---
 weed/command/iam.go          | 5 ++++-
 weed/iamapi/iamapi_server.go | 5 +++++
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/weed/command/iam.go b/weed/command/iam.go
index c484ed18d..8f4ac878d 100644
--- a/weed/command/iam.go
+++ b/weed/command/iam.go
@@ -76,7 +76,7 @@ func (iamopt *IamOptions) startIamServer() bool {
 
 	masters := pb.ServerAddresses(*iamopt.masters).ToAddressMap()
 	router := mux.NewRouter().SkipClean(true)
-	_, iamApiServer_err := iamapi.NewIamApiServer(router, &iamapi.IamServerOption{
+	iamApiServer, iamApiServer_err := iamapi.NewIamApiServer(router, &iamapi.IamServerOption{
 		Masters:        masters,
 		Filer:          filerAddress,
 		Port:           *iamopt.port,
@@ -86,6 +86,9 @@ func (iamopt *IamOptions) startIamServer() bool {
 	if iamApiServer_err != nil {
 		glog.Fatalf("IAM API Server startup error: %v", iamApiServer_err)
 	}
+	
+	// Ensure cleanup on shutdown
+	defer iamApiServer.Shutdown()
 
 	listenAddress := fmt.Sprintf(":%d", *iamopt.port)
 	iamApiListener, iamApiLocalListener, err := util.NewIpAndLocalListeners(*iamopt.ip, *iamopt.port, time.Duration(10)*time.Second)
diff --git a/weed/iamapi/iamapi_server.go b/weed/iamapi/iamapi_server.go
index d7fb1930d..361d9bec9 100644
--- a/weed/iamapi/iamapi_server.go
+++ b/weed/iamapi/iamapi_server.go
@@ -115,8 +115,13 @@ func (iama *IamApiServer) registerRouter(router *mux.Router) {
 // Shutdown gracefully stops the IAM API server and releases resources.
 // It cancels the master client connection goroutine and closes gRPC connections.
 // This method is safe to call multiple times.
+//
+// Note: This method is called via defer in weed/command/iam.go for best-effort cleanup.
+// For proper graceful shutdown on SIGTERM/SIGINT, signal handling should be added to
+// the command layer to call this method before process exit.
 func (iama *IamApiServer) Shutdown() {
 	if iama.shutdownCancel != nil {
+		glog.V(0).Infof("IAM API server shutting down, stopping master client connection")
 		iama.shutdownCancel()
 	}
 }