fix(distributed): keep remote unload idempotent, ask presence separately

2035a4d made UnloadRemoteModel return ErrRemoteModelNotLoaded when no node
holds the model, so ShutdownModel could answer 404 instead of a misleading
500. That narrowed a shared adapter contract to serve one caller and broke
the documented idempotent-unload guarantee, which CI caught on PR #10956:

  [FAIL] Node Backend Lifecycle (NATS-driven) > NATS backend.stop events
         should be no-op for models not on any node [Distributed]
         Expected success, but got: model not loaded on any node

The spec name states the contract outright. The matching unit assertion was
updated in that commit; this e2e one was missed because it lives under
tests/e2e/ with no build tags and does not run in package-scoped test runs.

Caller audit - who breaks when an idempotent unload becomes an error:

- pkg/model/watchdog.go:902 (LRU memory reclaimer) is the serious one. It
  untracks a model ONLY when shutdown returns nil or ErrModelNotFound. A new
  error type means the model is never untracked, so the reclaimer keeps
  re-selecting the same entry and never reclaims - a live wedge whenever a
  local store entry outlives the remote model.
- core/services/galleryop/managers_local.go:43 (DeleteModel) would warn on
  every deletion of an already-unloaded model.
- core/services/modeladmin/{state,config,remote_sync}.go stop instances
  best-effort against models that are frequently not loaded.
- deleteProcess itself: the no-local-process branch returns the unload result
  directly, so a stale local entry for a model no longer on any node turned a
  previously-successful cleanup into a failure.

Only ShutdownModel wants the distinction, and only on the local-store-miss
path. So the distinction moves to the caller instead of the contract:

- UnloadRemoteModel/UnloadRemoteModelContext return nil again when no node
  has the model, and ErrRemoteModelNotLoaded is removed.
- New optional RemoteModelPresenceChecker (HasRemoteModel) answers the
  question directly. deleteProcess consults it BEFORE unloading, because an
  idempotent unload cannot report afterwards whether anything was stopped.
  Absent locally AND cluster-wide is the only case that reports 404.
- A failed registry lookup is surfaced rather than reported as absence: an
  unreachable registry is not evidence a model is gone, and answering a
  confident 404 off a failed lookup is how an operator gets told a running
  model does not exist.
- Unloaders that predate the extension keep working - deleteProcess attempts
  the unload rather than refusing it - and compile-time assertions in the
  nodes package now pin all three optional interfaces, since both are
  consumed by runtime type assertion where drift degrades behavior silently
  instead of failing the build.

The contract is now pinned at both levels that disagreed, each spec pointing
at the other: "with no nodes returns nil" in unloader_test.go and "should be
no-op for models not on any node" in node_lifecycle_test.go.

Verified: full distributed e2e suite 233 passed / 0 failed (the suite that
failed 232/1 in CI); pkg/model and core/services/nodes green; make lint
new-from-merge-base reports 0 issues.

Assisted-by: Claude:claude-opus-4-8 golangci-lint
Signed-off-by: Ettore Di Giacinto <mudler@localai.io>

Author: mudler

core/services/nodes/unloader.go

@@ -72,6 +72,17 @@ func (a *RemoteUnloaderAdapter) InstallTimeout() time.Duration {
 	return a.installTimeout
 }
 
+// Compile-time proof that the adapter still satisfies the loader's optional
+// extensions. Both are consumed via runtime type assertion in deleteProcess, so
+// a signature drift here would silently downgrade behavior — losing force
+// propagation, or making ShutdownModel unable to tell a cluster-wide miss from
+// a completed unload — rather than failing the build.
+var (
+	_ model.RemoteModelUnloader        = (*RemoteUnloaderAdapter)(nil)
+	_ model.RemoteModelContextUnloader = (*RemoteUnloaderAdapter)(nil)
+	_ model.RemoteModelPresenceChecker = (*RemoteUnloaderAdapter)(nil)
+)
+
 // UnloadRemoteModel finds the node(s) hosting the given model and tells them
 // to stop their backend process via NATS backend.stop event.
 // The worker process handles a bounded Free() followed by process termination;
@@ -81,6 +92,22 @@ func (a *RemoteUnloaderAdapter) UnloadRemoteModel(modelName string) error {
 	return a.UnloadRemoteModelContext(context.Background(), modelName, false)
 }
 
+// HasRemoteModel reports whether any node currently holds the model. It exists
+// because UnloadRemoteModel is idempotent and so cannot signal "there was
+// nothing to stop"; ShutdownModel consults this first so it can answer 404 for
+// a model loaded neither locally nor anywhere in the cluster, instead of the
+// misleading 500 "model not found" that a local-store miss used to produce.
+func (a *RemoteUnloaderAdapter) HasRemoteModel(ctx context.Context, modelName string) (bool, error) {
+	if ctx == nil {
+		ctx = context.Background()
+	}
+	nodes, err := a.registry.FindNodesWithModel(ctx, modelName)
+	if err != nil {
+		return false, fmt.Errorf("finding nodes with model %q: %w", modelName, err)
+	}
+	return len(nodes) > 0, nil
+}
+
 // UnloadRemoteModelContext is the cancellation-aware extension used by the
 // model loader to preserve forced shutdown across the distributed boundary.
 func (a *RemoteUnloaderAdapter) UnloadRemoteModelContext(ctx context.Context, modelName string, force bool) error {
@@ -92,12 +119,12 @@ func (a *RemoteUnloaderAdapter) UnloadRemoteModelContext(ctx context.Context, mo
 		return fmt.Errorf("finding nodes with model %q: %w", modelName, err)
 	}
 	if len(nodes) == 0 {
-		// Distinguish "no node has it" from "stopped successfully" so the
-		// caller can report the truth to an operator. Returning nil here made
-		// a no-op stop indistinguishable from a real one, and left the loader
-		// unable to tell a cluster-wide miss from a completed unload.
+		// Unloading is idempotent by contract: cleanup paths (model deletion,
+		// config edits, watchdog eviction) legitimately run against an
+		// already-unloaded model and must not fail. Callers that need to tell
+		// this case apart use HasRemoteModel before unloading.
 		xlog.Debug("No remote nodes found with model", "model", modelName)
-		return model.ErrRemoteModelNotLoaded
+		return nil
 	}
 
 	var unloadErr error

core/services/nodes/unloader_test.go

@@ -14,7 +14,6 @@ import (
 
 	"github.com/mudler/LocalAI/core/services/galleryop"
 	"github.com/mudler/LocalAI/core/services/messaging"
-	"github.com/mudler/LocalAI/pkg/model"
 )
 
 // --- Fakes ---
@@ -127,14 +126,46 @@ var _ = Describe("RemoteUnloaderAdapter", func() {
 		adapter = NewRemoteUnloaderAdapter(locator, mc, 3*time.Minute, 15*time.Minute)
 	})
 
+	// HasRemoteModel carries the distinction that UnloadRemoteModel
+	// deliberately does not, so ShutdownModel can answer 404 for a model that
+	// is loaded neither locally nor anywhere in the cluster without making the
+	// shared unload path fail for every idempotent cleanup caller.
+	Describe("HasRemoteModel", func() {
+		It("reports false when no node has the model", func() {
+			locator.nodes = nil
+			loaded, err := adapter.HasRemoteModel(context.Background(), "my-model")
+			Expect(err).ToNot(HaveOccurred())
+			Expect(loaded).To(BeFalse())
+		})
+
+		It("reports true when a node has the model", func() {
+			locator.nodes = []BackendNode{{ID: "node-1", Name: "worker-1"}}
+			loaded, err := adapter.HasRemoteModel(context.Background(), "my-model")
+			Expect(err).ToNot(HaveOccurred())
+			Expect(loaded).To(BeTrue())
+		})
+
+		It("surfaces a registry failure instead of reporting absence", func() {
+			// An unreachable registry is not evidence that the model is gone;
+			// reporting false would let ShutdownModel answer a confident 404
+			// on the strength of a failed lookup.
+			locator.findErr = errors.New("registry unavailable")
+			_, err := adapter.HasRemoteModel(context.Background(), "my-model")
+			Expect(err).To(HaveOccurred())
+		})
+	})
+
 	Describe("UnloadRemoteModel", func() {
-		It("with no nodes reports the model is not loaded", func() {
-			// Previously returned nil, making "stopped it" and "there was
-			// nothing to stop" indistinguishable. ShutdownModel relies on
-			// this distinction to choose between success and a genuine 404,
-			// so a cluster-wide miss must be reported, not swallowed.
+		It("with no nodes returns nil", func() {
+			// Unloading is idempotent: cleanup paths (model deletion, config
+			// edits, watchdog eviction) legitimately run against an already
+			// unloaded model, and turning that into an error wedges the
+			// watchdog's LRU reclaimer, which only untracks a model when
+			// shutdown reports success. The same contract is pinned end to end
+			// by "should be no-op for models not on any node" in
+			// tests/e2e/distributed/node_lifecycle_test.go — keep them in step.
 			locator.nodes = nil
-			Expect(adapter.UnloadRemoteModel("my-model")).To(MatchError(model.ErrRemoteModelNotLoaded))
+			Expect(adapter.UnloadRemoteModel("my-model")).To(Succeed())
 			Expect(mc.published).To(BeEmpty())
 		})
 

pkg/model/loader.go

@@ -41,6 +41,20 @@ type RemoteModelContextUnloader interface {
 	UnloadRemoteModelContext(ctx context.Context, modelName string, force bool) error
 }
 
+// RemoteModelPresenceChecker reports whether any node in the cluster currently
+// holds the model.
+//
+// Unloading is deliberately idempotent: UnloadRemoteModel succeeds when no node
+// has the model, because cleanup paths (model deletion, config edits, watchdog
+// eviction) legitimately run against an already-unloaded model and must not
+// fail. That makes the unload result unable to distinguish "stopped it" from
+// "there was nothing to stop", so the one caller that needs the distinction —
+// ShutdownModel, which must report 404 rather than a misleading success for a
+// model loaded nowhere — asks separately, before unloading.
+type RemoteModelPresenceChecker interface {
+	HasRemoteModel(ctx context.Context, modelName string) (bool, error)
+}
+
 // ModelRouter is a callback that routes model loading to a remote node
 // instead of starting a local process. When set on the ModelLoader,
 // grpcModel() will delegate to this function before attempting local loading.

pkg/model/process.go

@@ -26,12 +26,6 @@ var (
 	// ErrModelBusy indicates that a graceful shutdown context ended while
 	// requests were still in flight.
 	ErrModelBusy = errors.New("model is still busy")
-
-	// ErrRemoteModelNotLoaded is returned by a RemoteModelUnloader when no
-	// node in the cluster has the model loaded. It exists so the local store
-	// miss and the cluster-wide miss stay distinguishable: only when BOTH are
-	// empty may we tell the operator the model is not loaded.
-	ErrRemoteModelNotLoaded = errors.New("model not loaded on any node")
 )
 
 const (
@@ -85,14 +79,22 @@ func (ml *ModelLoader) deleteProcess(ctx context.Context, s string, force bool)
 		// left its backend process untouched.
 		if remoteUnloader != nil {
 			xlog.Debug("Model not in local store; asking the remote unloader", "model", s)
-			if err := unloadRemote(ctx, remoteUnloader, s, force); err != nil {
-				if errors.Is(err, ErrRemoteModelNotLoaded) {
-					// Absent locally AND cluster-wide: genuinely not loaded.
+			// Ask BEFORE unloading. Unloading is idempotent by contract, so it
+			// cannot afterwards tell us whether anything was actually stopped,
+			// and only a model absent locally AND cluster-wide may be reported
+			// as not found.
+			if checker, ok := remoteUnloader.(RemoteModelPresenceChecker); ok {
+				loaded, err := checker.HasRemoteModel(ctx, s)
+				if err != nil {
+					// An unreachable registry is not evidence of absence;
+					// saying "not found" here would be a guess presented as fact.
+					return fmt.Errorf("checking whether model %q is loaded on any node: %w", s, err)
+				}
+				if !loaded {
 					return ErrModelNotFound
 				}
-				return err
 			}
-			return nil
+			return unloadRemote(ctx, remoteUnloader, s, force)
 		}
 		xlog.Debug("Model not found", "model", s)
 		return ErrModelNotFound

pkg/model/remote_shutdown_test.go

@@ -1,24 +1,48 @@
 package model_test
 
 import (
+	"context"
+	"errors"
+
 	"github.com/mudler/LocalAI/pkg/model"
 	"github.com/mudler/LocalAI/pkg/system"
 
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
 )
 
-// fakeRemoteUnloader records the models it was asked to unload so the specs
-// can assert the remote path was actually taken (not merely that no error
-// surfaced).
+// fakeRemoteUnloader records the models it was asked to unload so the specs can
+// assert the remote path was actually taken (not merely that no error
+// surfaced). It mirrors the real adapter's contract: unloading is idempotent
+// and reports nil even when nothing was loaded, so presence is a separate
+// question.
 type fakeRemoteUnloader struct {
-	called []string
-	err    error
+	called    []string
+	unloadErr error
+
+	present     bool
+	presenceErr error
+	asked       []string
 }
 
 func (f *fakeRemoteUnloader) UnloadRemoteModel(modelName string) error {
 	f.called = append(f.called, modelName)
-	return f.err
+	return f.unloadErr
+}
+
+func (f *fakeRemoteUnloader) HasRemoteModel(_ context.Context, modelName string) (bool, error) {
+	f.asked = append(f.asked, modelName)
+	return f.present, f.presenceErr
+}
+
+// unloaderWithoutPresence is a RemoteModelUnloader that does NOT implement
+// RemoteModelPresenceChecker, pinning the compatibility path for third-party
+// implementations of the older interface.
+type unloaderWithoutPresence struct{ called []string }
+
+func (u *unloaderWithoutPresence) UnloadRemoteModel(modelName string) error {
+	u.called = append(u.called, modelName)
+	return nil
 }
 
 // In distributed mode the authoritative record of "is this model loaded" is
@@ -42,6 +66,7 @@ var _ = Describe("ShutdownModel in distributed mode", func() {
 	})
 
 	It("delegates to the remote unloader when the model is not in the local store", func() {
+		unloader.present = true
 		modelLoader.SetRemoteUnloader(unloader)
 
 		err := modelLoader.ShutdownModel("longcat-video-avatar-1.5")
@@ -52,21 +77,48 @@ var _ = Describe("ShutdownModel in distributed mode", func() {
 			"stopping a model that is running on a worker must succeed, not report 'model not found'")
 	})
 
-	It("reports not-found only after the remote unloader confirms no node has it", func() {
-		unloader.err = model.ErrRemoteModelNotLoaded
+	It("reports not-found only after the registry confirms no node has it", func() {
+		unloader.present = false
 		modelLoader.SetRemoteUnloader(unloader)
 
 		err := modelLoader.ShutdownModel("never-loaded")
 
-		Expect(unloader.called).To(ConsistOf("never-loaded"),
+		Expect(unloader.asked).To(ConsistOf("never-loaded"),
 			"the registry must be consulted before declaring a model not found")
-		Expect(err).To(HaveOccurred(),
-			"a genuinely absent model must still error — silently succeeding would hide a failed stop")
+		Expect(err).To(MatchError(model.ErrModelNotFound),
+			"absent locally AND cluster-wide is the only case that may report not-found")
+		Expect(unloader.called).To(BeEmpty(),
+			"nothing to unload — no point publishing a stop for a model no node holds")
+	})
+
+	It("does not claim not-found when the registry lookup fails", func() {
+		// An unreachable registry is not evidence of absence. Reporting 404
+		// here would tell an operator the model is gone on the strength of a
+		// failed lookup.
+		unloader.presenceErr = errors.New("registry unavailable")
+		modelLoader.SetRemoteUnloader(unloader)
+
+		err := modelLoader.ShutdownModel("maybe-loaded")
+
+		Expect(err).To(HaveOccurred())
+		Expect(err).ToNot(MatchError(model.ErrModelNotFound))
+	})
+
+	It("still unloads via an unloader that cannot answer presence", func() {
+		// Older RemoteModelUnloader implementations have no presence check.
+		// They must keep working: attempt the unload rather than refusing it.
+		legacy := &unloaderWithoutPresence{}
+		modelLoader.SetRemoteUnloader(legacy)
+
+		err := modelLoader.ShutdownModel("some-model")
+
+		Expect(legacy.called).To(ConsistOf("some-model"))
+		Expect(err).ToNot(HaveOccurred())
 	})
 
 	It("still reports not-found when no remote unloader is configured", func() {
 		// Single-node behavior must be unchanged.
 		err := modelLoader.ShutdownModel("never-loaded")
-		Expect(err).To(HaveOccurred())
+		Expect(err).To(MatchError(model.ErrModelNotFound))
 	})
 })

tests/e2e/distributed/node_lifecycle_test.go

@@ -141,6 +141,15 @@ var _ = Describe("Node Backend Lifecycle (NATS-driven)", Label("Distributed"), f
 		})
 
 		It("should be no-op for models not on any node", func() {
+			// Unloading is idempotent by contract: cleanup paths (model
+			// deletion, config edits, watchdog eviction) legitimately run
+			// against an already-unloaded model, and the watchdog's LRU
+			// reclaimer only untracks a model when shutdown reports success.
+			// Callers needing to tell "stopped it" from "nothing to stop" —
+			// ShutdownModel, so it can answer 404 — use HasRemoteModel instead.
+			// The same contract is pinned at unit level by "with no nodes
+			// returns nil" in core/services/nodes/unloader_test.go; keep them
+			// in step.
 			adapter := nodes.NewRemoteUnloaderAdapter(registry, infra.NC, 3*time.Minute, 15*time.Minute)
 			Expect(adapter.UnloadRemoteModel("nonexistent-model")).To(Succeed())
 		})