Fix management reload snapshot ordering

Author: LTbinglingfeng

internal/api/handlers/management/handler.go

@@ -38,25 +38,33 @@ const attemptMaxIdleTime = 2 * time.Hour
 
 // Handler aggregates config reference, persistence path and helpers.
 type Handler struct {
-	cfg                    *config.Config
-	configFilePath         string
-	mu                     sync.Mutex
-	attemptsMu             sync.Mutex
-	failedAttempts         map[string]*attemptInfo // keyed by client IP
-	authManager            *coreauth.Manager
-	tokenStore             coreauth.Store
-	localPassword          string
-	allowRemoteOverride    bool
-	envSecret              string
-	logDir                 string
-	postAuthHook           coreauth.PostAuthHook
-	postAuthPersistHook    coreauth.PostAuthHook
-	pluginHost             *pluginhost.Host
-	configReloadHook       func(context.Context, *config.Config)
-	pluginStoreRegistryURL string
-	pluginStoreHTTPClient  pluginstore.HTTPDoer
-	pluginReleaseCacheMu   sync.Mutex
-	pluginReleaseCache     map[string]pluginReleaseCacheEntry
+	cfg                     *config.Config
+	configFilePath          string
+	mu                      sync.Mutex
+	reloadMu                sync.Mutex
+	reloadGeneration        uint64
+	appliedReloadGeneration uint64
+	attemptsMu              sync.Mutex
+	failedAttempts          map[string]*attemptInfo // keyed by client IP
+	authManager             *coreauth.Manager
+	tokenStore              coreauth.Store
+	localPassword           string
+	allowRemoteOverride     bool
+	envSecret               string
+	logDir                  string
+	postAuthHook            coreauth.PostAuthHook
+	postAuthPersistHook     coreauth.PostAuthHook
+	pluginHost              *pluginhost.Host
+	configReloadHook        func(context.Context, *config.Config)
+	pluginStoreRegistryURL  string
+	pluginStoreHTTPClient   pluginstore.HTTPDoer
+	pluginReleaseCacheMu    sync.Mutex
+	pluginReleaseCache      map[string]pluginReleaseCacheEntry
+}
+
+type configReloadSnapshot struct {
+	cfg        *config.Config
+	generation uint64
 }
 
 // NewHandler creates a new management handler instance.
@@ -152,48 +160,63 @@ func (h *Handler) SetConfigReloadHook(hook func(context.Context, *config.Config)
 	h.mu.Unlock()
 }
 
-// snapshotConfigLocked clones the full runtime config while h.mu is held.
+// reloadSnapshotConfigLocked clones the runtime config and assigns a reload generation.
 // Callers must hold h.mu.
-func (h *Handler) snapshotConfigLocked() *config.Config {
+func (h *Handler) reloadSnapshotConfigLocked() configReloadSnapshot {
 	if h == nil || h.cfg == nil {
-		return nil
+		return configReloadSnapshot{}
+	}
+	h.reloadGeneration++
+	return configReloadSnapshot{
+		cfg:        h.cfg.CloneForRuntime(),
+		generation: h.reloadGeneration,
 	}
-	return h.cfg.CloneForRuntime()
 }
 
 // saveConfigAndSnapshotLocked saves h.cfg and returns a full runtime config snapshot.
 // Callers must hold h.mu.
-func (h *Handler) saveConfigAndSnapshotLocked(c *gin.Context) (*config.Config, bool) {
+func (h *Handler) saveConfigAndSnapshotLocked(c *gin.Context) (configReloadSnapshot, bool) {
 	if errSave := config.SaveConfigPreserveComments(h.configFilePath, h.cfg); errSave != nil {
 		c.JSON(http.StatusInternalServerError, gin.H{"error": fmt.Sprintf("failed to save config: %v", errSave)})
-		return nil, false
+		return configReloadSnapshot{}, false
 	}
-	return h.snapshotConfigLocked(), true
+	return h.reloadSnapshotConfigLocked(), true
 }
 
 // reloadConfigAfterManagementSave reloads from an independent config snapshot.
 // Callers must pass a full Config clone captured immediately after a successful save.
-func (h *Handler) reloadConfigAfterManagementSave(ctx context.Context, cfgSnapshot *config.Config) {
-	if h == nil || cfgSnapshot == nil {
+func (h *Handler) reloadConfigAfterManagementSave(ctx context.Context, snapshot configReloadSnapshot) {
+	if h == nil || snapshot.cfg == nil || snapshot.generation == 0 {
 		return
 	}
+	h.reloadMu.Lock()
+	defer h.reloadMu.Unlock()
+
 	h.mu.Lock()
+	if snapshot.generation < h.appliedReloadGeneration {
+		h.mu.Unlock()
+		return
+	}
 	hook := h.configReloadHook
 	host := h.pluginHost
 	h.mu.Unlock()
 	if hook != nil {
-		hook(ctx, cfgSnapshot)
-		return
+		hook(ctx, snapshot.cfg)
+	} else if host != nil {
+		host.ApplyConfig(ctx, snapshot.cfg)
 	}
-	if host != nil {
-		host.ApplyConfig(ctx, cfgSnapshot)
+
+	h.mu.Lock()
+	if snapshot.generation > h.appliedReloadGeneration {
+		h.appliedReloadGeneration = snapshot.generation
 	}
+	h.mu.Unlock()
 }
 
 // reloadConfigAfterManagementSaveAsync reloads from an independent config snapshot.
 // Callers must pass a full Config clone captured immediately after a successful save.
-func (h *Handler) reloadConfigAfterManagementSaveAsync(ctx context.Context, cfgSnapshot *config.Config) {
-	if h == nil || cfgSnapshot == nil {
+func (h *Handler) reloadConfigAfterManagementSaveAsync(ctx context.Context, snapshot configReloadSnapshot) {
+	if h == nil || snapshot.cfg == nil || snapshot.generation == 0 {
 		return
 	}
 	reloadCtx := context.Background()
@@ -206,7 +229,7 @@ func (h *Handler) reloadConfigAfterManagementSaveAsync(ctx context.Context, cfgS
 				log.WithField("panic", recovered).Error("management: async config reload panicked")
 			}
 		}()
-		h.reloadConfigAfterManagementSave(reloadCtx, cfgSnapshot)
+		h.reloadConfigAfterManagementSave(reloadCtx, snapshot)
 	}()
 }
 

internal/api/handlers/management/plugin_store.go

@@ -226,7 +226,7 @@ func (h *Handler) installPluginFromStore(c *gin.Context, goos, goarch string) {
 	if errInstall != nil {
 		if unloadedBeforeWrite {
 			h.mu.Lock()
-			cfgSnapshot := h.snapshotConfigLocked()
+			cfgSnapshot := h.reloadSnapshotConfigLocked()
 			h.mu.Unlock()
 			h.reloadConfigAfterManagementSave(c.Request.Context(), cfgSnapshot)
 		}
@@ -271,7 +271,7 @@ func (h *Handler) installPluginFromStore(c *gin.Context, goos, goarch string) {
 		})
 		return
 	}
-	cfgSnapshot := h.snapshotConfigLocked()
+	cfgSnapshot := h.reloadSnapshotConfigLocked()
 	h.mu.Unlock()
 
 	h.reloadConfigAfterManagementSaveAsync(c.Request.Context(), cfgSnapshot)

internal/api/handlers/management/plugins.go

@@ -373,7 +373,7 @@ func (h *Handler) DeletePlugin(c *gin.Context) {
 			return
 		}
 	}
-	cfgSnapshot := h.snapshotConfigLocked()
+	cfgSnapshot := h.reloadSnapshotConfigLocked()
 	h.mu.Unlock()
 
 	h.reloadConfigAfterManagementSaveAsync(c.Request.Context(), cfgSnapshot)

internal/api/handlers/management/plugins_test.go

@@ -51,6 +51,39 @@ func captureConfigReload(h *Handler) (<-chan *config.Config, <-chan struct{}) {
 	return reloads, done
 }
 
+func TestConfigReloadGenerationSkipsOlderSnapshot(t *testing.T) {
+	t.Parallel()
+
+	h := &Handler{
+		cfg: &config.Config{
+			Plugins: config.PluginsConfig{
+				Configs: map[string]config.PluginInstanceConfig{
+					"sample": pluginConfigFromYAML(t, "enabled: true\nmode: old\n"),
+				},
+			},
+		},
+	}
+	reloadedModes := make([]string, 0, 1)
+	h.SetConfigReloadHook(func(_ context.Context, cfg *config.Config) {
+		reloadedModes = append(reloadedModes, pluginRawScalarValue(t, cfg.Plugins.Configs["sample"], "mode"))
+	})
+
+	h.mu.Lock()
+	older := h.reloadSnapshotConfigLocked()
+	item := h.cfg.Plugins.Configs["sample"]
+	setPluginRawScalarValue(t, &item.Raw, "mode", "new")
+	h.cfg.Plugins.Configs["sample"] = item
+	newer := h.reloadSnapshotConfigLocked()
+	h.mu.Unlock()
+
+	h.reloadConfigAfterManagementSave(context.Background(), newer)
+	h.reloadConfigAfterManagementSave(context.Background(), older)
+
+	if len(reloadedModes) != 1 || reloadedModes[0] != "new" {
+		t.Fatalf("reloaded modes = %#v, want only new snapshot", reloadedModes)
+	}
+}
+
 func TestListPluginsIncludesScannedAndConfiguredPlugins(t *testing.T) {
 	t.Parallel()