From 5c9d8190d9b056935f135fac71b834816c95eea8 Mon Sep 17 00:00:00 2001 From: Ettore Di Giacinto Date: Mon, 24 Aug 2026 06:40:30 +0000 Subject: [PATCH] fix(distributed): resync revisions after the configs are loaded The resync added in 3953448f6 ran before LoadModelConfigsFromPath, so it read an empty loader, reconciled nothing and reported success. The symptom was a stored revision that stayed stale across restarts while the log showed no complaint, which is exactly what the resync was meant to prevent. Move the call after the configs are loaded, and refuse to treat an empty loader as a clean run: reconciling zero models is indistinguishable from reconciling correctly, and that is what hid the mis-ordered call. Signed-off-by: Ettore Di Giacinto Assisted-by: Claude Code:claude-opus-5 [golangci-lint] --- core/application/startup.go | 29 ++++++++++++------- core/services/modeladmin/revision_resync.go | 11 ++++++- .../modeladmin/revision_resync_test.go | 15 ++++++++++ 3 files changed, 44 insertions(+), 11 deletions(-) diff --git a/core/application/startup.go b/core/application/startup.go index a5aec50908c7..12217e42687f 100644 --- a/core/application/startup.go +++ b/core/application/startup.go @@ -267,6 +267,10 @@ func New(opts ...config.AppOption) (*Application, error) { } // Initialize distributed mode services (NATS, object storage, node registry) + // revisionStore is built inside the distributed block below but used after + // the model configs are loaded, so it is declared out here. + var revisionStore modeladmin.RevisionStore + distSvc, err := initDistributed(options, application.authDB, application.ModelConfigLoader()) if err != nil { return nil, fmt.Errorf("distributed mode initialization failed: %w", err) @@ -373,16 +377,9 @@ func New(opts ...config.AppOption) (*Application, error) { cfgLoaderOpts := options.ToConfigLoaderOptions() modelRevisionLifecycle := modeladmin.NewDistributedModelRevisionLifecycle(distSvc.Registry, distSvc.ModelCleanup) gs.SetModelRevisionLifecycle(modelRevisionLifecycle) - // Bring the controller's stored revisions back in line with the - // configuration on disk. An inference request may only establish a - // revision, never replace one, so a model whose stored value had - // drifted stayed unroutable until someone deleted the row. - if err := modeladmin.ResyncModelConfigRevisions(options.Context, - application.ModelConfigLoader(), - modeladmin.NewRevisionStore(distSvc.Registry, modelRevisionLifecycle), - ); err != nil { - xlog.Warn("Failed to resync model config revisions", "error", err) - } + // Captured here, used after the model configs are loaded below: the + // resync reads the loader, which is still empty at this point. + revisionStore = modeladmin.NewRevisionStore(distSvc.Registry, modelRevisionLifecycle) gs.OnModelsChanged = func(evt messaging.CacheInvalidateEvent) { // ApplyRemoteChange honors the op: a "delete" prunes the element // (a reload-from-path is additive and cannot drop it), anything @@ -429,6 +426,18 @@ func New(opts ...config.AppOption) (*Application, error) { xlog.Error("error loading config files", "error", err) } + // Bring the controller's stored revisions back in line with the + // configuration just loaded. An inference request may only establish a + // revision, never replace one, so a model whose stored value has drifted + // stays unroutable until something republishes it. This has to run after + // the load above: the loader is empty until then, and a resync against an + // empty loader silently reconciles nothing. + if revisionStore != nil { + if err := modeladmin.ResyncModelConfigRevisions(options.Context, application.ModelConfigLoader(), revisionStore); err != nil { + xlog.Warn("Failed to resync model config revisions", "error", err) + } + } + if err := gallery.RegisterBackends(options.SystemState, application.ModelLoader()); err != nil { xlog.Error("error registering external backends", "error", err) } diff --git a/core/services/modeladmin/revision_resync.go b/core/services/modeladmin/revision_resync.go index 4724f60e03b0..1eceff3f9529 100644 --- a/core/services/modeladmin/revision_resync.go +++ b/core/services/modeladmin/revision_resync.go @@ -70,8 +70,17 @@ func ResyncModelConfigRevisions(ctx context.Context, loader *config.ModelConfigL return nil } + configs := loader.GetAllModelsConfigs() + if len(configs) == 0 { + // Reconciling nothing is indistinguishable from reconciling correctly, + // which is how a caller that ran this before the configs were loaded + // went unnoticed. Say so rather than report success. + xlog.Warn("Skipping model config revision resync: no model configurations are loaded") + return nil + } + var transitions []ModelRevisionTransition - for _, cfg := range loader.GetAllModelsConfigs() { + for _, cfg := range configs { want, err := config.ModelConfigRevision(&cfg) if err != nil { return fmt.Errorf("compute config revision for %q: %w", cfg.Name, err) diff --git a/core/services/modeladmin/revision_resync_test.go b/core/services/modeladmin/revision_resync_test.go index 3979ccf75298..12772544245a 100644 --- a/core/services/modeladmin/revision_resync_test.go +++ b/core/services/modeladmin/revision_resync_test.go @@ -140,3 +140,18 @@ var _ = Describe("ResyncModelConfigRevisions", func() { Expect(store.applied).To(BeEmpty()) }) }) + +// Running the resync before the model configs are loaded reconciled nothing +// while reporting success, which is how a mis-ordered startup call went +// unnoticed. An empty loader is now called out instead of looking like a +// clean run. +var _ = Describe("ResyncModelConfigRevisions with nothing loaded", func() { + It("does not touch stored revisions when no configs are loaded", func() { + dir := GinkgoT().TempDir() + loader := config.NewModelConfigLoader(dir) + store := &stubRevisionStore{stored: map[string]string{"served-before": "stale"}} + + Expect(ResyncModelConfigRevisions(context.Background(), loader, store)).To(Succeed()) + Expect(store.applied).To(BeEmpty()) + }) +})