engine-manager: let the uninstaller keep a model store the user moved

The uninstaller took only the port from a user's override. The running
engine manager reads the whole override, so a reset kept a models_dir
the user had pointed elsewhere while the uninstaller kept only the
bundled default -- and removed the user's store if it sat inside a
removal target.

Take models_dir along with the port. Both say where the engine's things
are rather than what to run or delete: a store decides only what
removal keeps. The overlay is checked with the same validation startup
applies, so a store that would hold a removal target, or a templated
one, is ignored whole.

Signed-off-by: Terve <ntervalon@nvidia.com>

Author: Noah-Tervalon-Nvidia

services/nvpair-engine-manager/main.go

@@ -168,7 +168,7 @@ func userPaths() (manifestDir, installBase string) {
 func runUninstallManaged() {
 	manifestDir, installBase := userPaths()
 	reg := buildBundledRegistry()
-	reg.applyPortOverrides(manifestDir)
+	reg.applyLocationOverrides(manifestDir)
 	exec := NewExecutor(reg, NewReporter(nil), func(string, any) {}, installBase)
 	ctx, cancel := context.WithTimeout(context.Background(), uninstallManagedTimeout)
 	defer cancel()

services/nvpair-engine-manager/portoverride_test.go

@@ -159,20 +159,29 @@ func TestWriteJSONAtomicReplacesExistingFile(t *testing.T) {
 	}
 }
 
-// TestUninstallerTakesOnlyThePortFromAnOverride checks the uninstaller looks for
-// an engine on the port the user moved it to, and takes nothing else from the
-// override. The uninstaller runs elevated on Windows, and the override is a file
-// any process running as the user can write.
-func TestUninstallerTakesOnlyThePortFromAnOverride(t *testing.T) {
-	host := runtime.GOOS + "/" + runtime.GOARCH
-	bundled, ok := buildBundledRegistry().Get("ollama")
+// bundledHostPlatform returns an engine's bundled platform for this host, or
+// skips the test where the engine has none.
+func bundledHostPlatform(t *testing.T, engine string) Platform {
+	t.Helper()
+	bundled, ok := buildBundledRegistry().Get(engine)
 	if !ok {
-		t.Fatal("no bundled ollama manifest")
+		t.Fatalf("no bundled %s manifest", engine)
 	}
-	want, ok := bundled.Platforms[host]
+	platform, ok := bundled.Platforms[runtime.GOOS+"/"+runtime.GOARCH]
 	if !ok {
-		t.Skipf("ollama has no manifest for %s", host)
+		t.Skipf("%s has no manifest for this host", engine)
 	}
+	return platform
+}
+
+// TestUninstallerTakesOnlyLocationsFromAnOverride checks the uninstaller looks
+// for an engine on the port the user moved it to and keeps the model store they
+// moved it to, and takes nothing else from the override. The uninstaller runs
+// elevated on Windows, and the override is a file any process running as the
+// user can write.
+func TestUninstallerTakesOnlyLocationsFromAnOverride(t *testing.T) {
+	host := runtime.GOOS + "/" + runtime.GOARCH
+	want := bundledHostPlatform(t, "ollama")
 	dir := t.TempDir()
 	override := map[string]any{
 		"engine":  "ollama",
@@ -190,7 +199,7 @@ func TestUninstallerTakesOnlyThePortFromAnOverride(t *testing.T) {
 	}
 
 	reg := buildBundledRegistry()
-	reg.applyPortOverrides(dir)
+	reg.applyLocationOverrides(dir)
 	got, ok := reg.Get("ollama")
 	if !ok {
 		t.Fatal("ollama vanished from the registry")
@@ -200,8 +209,41 @@ func TestUninstallerTakesOnlyThePortFromAnOverride(t *testing.T) {
 	if platform.Runtime.Port != 21002 {
 		t.Errorf("port %d, want the host platform's override, 21002", platform.Runtime.Port)
 	}
-	if platform.ModelsDir != want.ModelsDir || !reflect.DeepEqual(platform.Uninstall, want.Uninstall) {
-		t.Errorf("took more than the port from the override: models_dir %q, uninstall %+v",
-			platform.ModelsDir, platform.Uninstall)
+	if platform.ModelsDir != "~/somewhere-else" {
+		t.Errorf("models_dir %q, want the override's store", platform.ModelsDir)
+	}
+	if !reflect.DeepEqual(platform.Uninstall, want.Uninstall) {
+		t.Errorf("took the uninstall commands from the override: %+v", platform.Uninstall)
+	}
+}
+
+// TestUninstallerIgnoresAnOverrideStoreHoldingWhatItRemoves checks an override
+// whose model store would contain a removal target is ignored whole, as startup
+// ignores it, rather than leaving the uninstaller with a store it must refuse
+// to remove around.
+func TestUninstallerIgnoresAnOverrideStoreHoldingWhatItRemoves(t *testing.T) {
+	host := runtime.GOOS + "/" + runtime.GOARCH
+	want := bundledHostPlatform(t, "lmstudio")
+	dir := t.TempDir()
+	override := map[string]any{
+		"engine":     "lmstudio",
+		"models_dir": "~",
+		"runtime":    map[string]any{"port": 21003},
+	}
+	if err := writeJSONAtomic(filepath.Join(dir, "lmstudio.json"), override); err != nil {
+		t.Fatal(err)
+	}
+
+	reg := buildBundledRegistry()
+	reg.applyLocationOverrides(dir)
+	got, ok := reg.Get("lmstudio")
+	if !ok {
+		t.Fatal("lmstudio vanished from the registry")
+	}
+	platform := got.Platforms[host]
+
+	if platform.ModelsDir != want.ModelsDir || platform.Runtime.Port != want.Runtime.Port {
+		t.Errorf("applied an invalid override: models_dir %q, port %d; want the bundled %q, %d",
+			platform.ModelsDir, platform.Runtime.Port, want.ModelsDir, want.Runtime.Port)
 	}
 }

services/nvpair-engine-manager/registry.go

@@ -468,12 +468,15 @@ func (r *Registry) LoadOverrideDir(dir string) error {
 	return nil
 }
 
-// applyPortOverrides takes each engine's port from the user's override
-// manifest and nothing else from it. The uninstaller runs with privileges the
-// override's author may not have, so the commands it runs and the paths it
-// deletes stay bundled. A port decides only where it looks for the engine still
-// running, which is wherever the user moved it to.
-func (r *Registry) applyPortOverrides(dir string) {
+// applyLocationOverrides takes from each user override only where the engine's
+// things are: its port and its model store. The uninstaller runs with
+// privileges the override's author may not have, so the commands it runs and
+// the paths it deletes stay bundled. A port decides where it looks for the
+// engine still running, and a model store decides what removal keeps — the
+// same store the running engine manager, which reads the whole override,
+// already keeps. An override that would leave the platform invalid is ignored,
+// as it is at startup.
+func (r *Registry) applyLocationOverrides(dir string) {
 	if dir == "" {
 		return
 	}
@@ -488,23 +491,39 @@ func (r *Registry) applyPortOverrides(dir string) {
 		}
 		var override struct {
 			Engine    string   `json:"engine"`
+			ModelsDir string   `json:"models_dir"`
 			Runtime   portOnly `json:"runtime"`
 			Platforms map[string]struct {
-				Runtime portOnly `json:"runtime"`
+				ModelsDir string   `json:"models_dir"`
+				Runtime   portOnly `json:"runtime"`
 			} `json:"platforms"`
 		}
 		if err := json.Unmarshal(data, &override); err != nil || override.Engine != name {
 			continue
 		}
+		platform, ok := m.Platforms[host]
+		if !ok {
+			continue
+		}
+		hostOverride := override.Platforms[host]
 		port := override.Runtime.Port
-		if hostPort := override.Platforms[host].Runtime.Port; hostPort != 0 {
-			port = hostPort
+		if hostOverride.Runtime.Port != 0 {
+			port = hostOverride.Runtime.Port
 		}
-		platform, ok := m.Platforms[host]
-		if !ok || port <= 0 || port > 65535 {
+		if port > 0 && port <= 65535 {
+			platform.Runtime.Port = port
+		}
+		store := override.ModelsDir
+		if hostOverride.ModelsDir != "" {
+			store = hostOverride.ModelsDir
+		}
+		if store != "" {
+			platform.ModelsDir = store
+		}
+		if err := platform.validate(host); err != nil {
+			slog.Warn("ignoring an override's port and model store", "engine", name, "err", err)
 			continue
 		}
-		platform.Runtime.Port = port
 		m.Platforms[host] = platform
 	}
 }