From a3c87ceeb455612f1dfc7052868d61288af29bdd Mon Sep 17 00:00:00 2001 From: LTbinglingfeng Date: Wed, 17 Jun 2026 03:17:56 +0800 Subject: [PATCH] Fix management reload snapshot ordering --- internal/api/handlers/management/handler.go | 93 ++++++++++++------- .../api/handlers/management/plugin_store.go | 4 +- internal/api/handlers/management/plugins.go | 2 +- .../api/handlers/management/plugins_test.go | 33 +++++++ 4 files changed, 94 insertions(+), 38 deletions(-) diff --git a/internal/api/handlers/management/handler.go b/internal/api/handlers/management/handler.go index 3e83faf5..c5b6daa6 100644 --- a/internal/api/handlers/management/handler.go +++ b/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) }() } diff --git a/internal/api/handlers/management/plugin_store.go b/internal/api/handlers/management/plugin_store.go index fc13cdfe..0217cf5f 100644 --- a/internal/api/handlers/management/plugin_store.go +++ b/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) diff --git a/internal/api/handlers/management/plugins.go b/internal/api/handlers/management/plugins.go index b1afb822..3a77d130 100644 --- a/internal/api/handlers/management/plugins.go +++ b/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) diff --git a/internal/api/handlers/management/plugins_test.go b/internal/api/handlers/management/plugins_test.go index a03b217d..a07d54d8 100644 --- a/internal/api/handlers/management/plugins_test.go +++ b/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()