fix(engine-manager): fail closed on unpinned downloads and escalate engine stop

services/nvpair-engine-manager/README.md

@@ -115,24 +115,32 @@ paired engine-control surface relays settings to its local broker and streams
 full authoritative snapshots to pinned peers.
 
 ```
-NotInstalled --engine:install--> (HTTPS download + verify-if-pinned + user-mode run) --> Stopped
+NotInstalled --engine:install--> (HTTPS download + sha256 verify, fail closed + user-mode run) --> Stopped
 Stopped      --engine:start----> (adopt if already serving the port, else spawn) --> Running --health--> Running
-Running      --engine:stop-----> (stop signal, wait for exit; no timeout) --> Stopped
+Running      --engine:stop-----> (stop signal, wait up to the manifest grace, then SIGKILL/pgid) --> Stopped
 ```
 
 Detect uses the manifest's `detect` paths. Install is one-shot and
 user-mode — an HTTPS download, verified against the manifest's `sha256`
-when one is pinned (an unpinned fetch runs with a loud warning). Start
+pin. A manifest without a pin fails the install closed (any executed
+artifact must be pinned in the reviewed manifest); the escape hatch is
+`NVPAIR_ALLOW_UNPINNED_DOWNLOADS=1`, which runs the unpinned fetch with a
+loud warning instead. The bundled Ollama and LM Studio fetch URLs are
+rolling, so their manifests carry no pin and install through that escape
+hatch; the check exists so a manifest can opt in to verification by adding
+a `sha256`. Start
 waits for the readiness probe, then runs a periodic health probe; an
 unexpected exit is reported. The bundled Ollama manifest allows up to ten
 minutes for startup because GPU discovery can exceed the previous 30-second
 allowance on supported Windows systems. The deadline remains finite: if Ollama
 never serves its readiness endpoint, engine-manager stops the owned process and
 reports the failed start. Stop sends one stop signal and waits for the engine
-to exit, with no timeout: SIGTERM to the process group on Unix (graceful, no
-SIGKILL escalation), and `taskkill /T /F` on Windows — where the windowless
-engines we spawn can't receive a graceful (non-`/F`) close, so a forced
-terminate is the only signal that actually stops them.
+to exit up to the manifest's stop grace (default 5s, `grace_s` from the
+manifest's stop spec): SIGTERM to the process group on Unix, `taskkill /T /F`
+on Windows — where the windowless engines we spawn can't receive a graceful
+(non-`/F`) close, so a forced terminate is the only signal that actually stops
+them. An engine still alive after the grace is escalated to a forced kill
+(SIGKILL/pgid on Unix) so a hung engine cannot block shutdown forever.
 
 ### Adoption — start may attach to an engine it didn't launch
 

services/nvpair-engine-manager/install.go

@@ -293,19 +293,34 @@ func (e *Executor) download(ctx context.Context, engine string, f *Fetch) (strin
 	if want := strings.TrimSpace(f.SHA256); want != "" {
 		if !strings.EqualFold(sum, want) {
 			os.Remove(tmp.Name())
-			return "", fmt.Errorf("checksum mismatch for %s: got %s, want %s", f.URL, sum, want)
+			return "", fmt.Errorf("checksum mismatch for %s: got %s, want %s; "+
+				"if the vendor republished this artifact, update the manifest pin, "+
+				"or set NVPAIR_ALLOW_UNPINNED_DOWNLOADS=1 to accept unverified downloads",
+				f.URL, sum, want)
 		}
-	} else {
-		// Unpinned download: bytes are not integrity-checked, only
-		// transport-secured (HTTPS, enforced above) — the same weaker
-		// guarantee as a `script` install. Logged loudly, and the computed
-		// digest is surfaced so a manifest author can pin it later.
-		slog.Warn("UNPINNED download: manifest has no sha256, integrity not verified",
+	} else if allowUnpinnedDownloads() {
+		// Explicit operator opt-in: bytes are not integrity-checked, only
+		// transport-secured (HTTPS, enforced above). Logged loudly, and the
+		// computed digest is surfaced so a manifest author can pin it later.
+		slog.Warn("UNPINNED download allowed by NVPAIR_ALLOW_UNPINNED_DOWNLOADS: integrity not verified",
 			"engine", engine, "url", f.URL, "computed_sha256", sum)
+	} else {
+		// Fail closed: an executed artifact without a pinned digest means any
+		// HTTPS host serving the manifest URL yields code execution. Surface
+		// the digest so the manifest author can pin it immediately.
+		os.Remove(tmp.Name())
+		return "", fmt.Errorf("download %s has no sha256 pin in the manifest (computed sha256 %s); "+
+			"pin it or set NVPAIR_ALLOW_UNPINNED_DOWNLOADS=1 to accept unverified downloads", f.URL, sum)
 	}
 	return tmp.Name(), nil
 }
 
+// allowUnpinnedDownloads reports whether the operator explicitly opted in to
+// executing downloads whose manifests carry no sha256 pin.
+func allowUnpinnedDownloads() bool {
+	return os.Getenv("NVPAIR_ALLOW_UNPINNED_DOWNLOADS") == "1"
+}
+
 // runCommand executes a manifest-declared argv (an install or uninstall
 // step), hiding the console window on Windows; on failure it returns the
 // combined output for diagnostics.

services/nvpair-engine-manager/launch_test.go

@@ -97,7 +97,7 @@ func TestLaunchTextReachesChildLiterally(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
-	t.Cleanup(proc.stop)
+	t.Cleanup(func() { proc.stop(Runtime{}) })
 	select {
 	case <-proc.done:
 	case <-time.After(10 * time.Second):

services/nvpair-engine-manager/lifecycle.go

@@ -277,7 +277,7 @@ func (e *Executor) bringUpProcess(ctx context.Context, st *engineState, engine s
 		st.mu.Lock()
 		st.stopping = true
 		st.mu.Unlock()
-		proc.stop()
+		proc.stop(rt)
 		st.mu.Lock()
 		st.proc = nil
 		st.mu.Unlock()
@@ -496,7 +496,7 @@ func (e *Executor) doStop(st *engineState, engine string) error {
 			return e.reconcileFailedCommandStop(st, engine, rt.Ready == nil || !e.waitUnavailable(rt.Ready, port, time.Second), err)
 		}
 	} else if proc != nil {
-		proc.stop()
+		proc.stop(rt)
 	}
 
 	e.markStopped(st, engine)

services/nvpair-engine-manager/proc.go

@@ -6,6 +6,7 @@ package main
 import (
 	"bufio"
 	"io"
+	"log/slog"
 	"os"
 	"os/exec"
 	"path/filepath"
@@ -14,6 +15,12 @@ import (
 	"time"
 )
 
+// stopForceWait bounds the final wait after a forced kill. A forced kill
+// normally reaps the process immediately, so this only matters when the kill
+// itself fails (an elevated child the signal cannot reach); without a bound,
+// stop would hang shutdown forever.
+const stopForceWait = 5 * time.Second
+
 // managedProc is a spawned engine process with its stdout/stderr
 // captured line-by-line and a `done` channel that closes when it
 // exits. The OS-specific primitives it relies on (console hiding,
@@ -75,21 +82,23 @@ func scanLines(r io.Reader, stream string, onLine func(stream, line string)) {
 	}
 }
 
-// stop stops the process and waits for it to exit, with no timeout.
+// stop stops the process and waits for it to exit.
 //
-// It sends one platform-appropriate stop signal (see gracefulSignal) and then
-// blocks until the process is gone:
-//   - Unix: SIGTERM to the process group — a graceful ask, with no escalation
-//     to SIGKILL. A well-behaved engine (Ollama, and the test fake) exits on it.
+// It sends one platform-appropriate stop signal (see gracefulSignal), waits
+// stopGrace(rt) (the manifest's stop spec, default 5s) for the engine to
+// honor it, and then escalates to a forced kill of the whole tree/group (see
+// forceSignal):
+//   - Unix: SIGTERM to the process group, then SIGKILL to the group if the
+//     engine is still alive after the grace period. The group kill reaches
+//     engines that forked helper processes (model runners, etc.).
 //   - Windows: taskkill /T /F. Our engines run windowless, and a windowless
-//     process can't receive a graceful (non-/F) close, so /F is the only signal
-//     that actually stops it — never force-killing there would leave the engine
-//     running forever.
+//     process can't receive a graceful (non-/F) close, so /F is the only
+//     signal that actually stops it — never force-killing there would leave
+//     the engine running forever.
 //
-// There is deliberately no timeout: a stop is complete only when the engine has
-// actually exited. On Unix an engine that ignored SIGTERM would not be stopped
-// and this would wait for it; in practice engines exit on SIGTERM.
-func (mp *managedProc) stop() {
+// A stop is complete only when the engine has actually exited, so after
+// escalation stop still blocks on the exit rather than returning early.
+func (mp *managedProc) stop(rt Runtime) {
 	if mp == nil || mp.cmd == nil || mp.cmd.Process == nil {
 		return
 	}
@@ -99,7 +108,26 @@ func (mp *managedProc) stop() {
 	default:
 	}
 	_ = gracefulSignal(mp.cmd)
-	<-mp.done
+	grace := time.NewTimer(stopGrace(rt))
+	defer grace.Stop()
+	select {
+	case <-mp.done:
+		return
+	case <-grace.C:
+	}
+	// The engine ignored the graceful signal: force the whole group. SIGKILL
+	// cannot be caught, so the process-exit goroutine will observe the exit
+	// and close done. Bound the final wait too, so a forced kill that itself
+	// fails (an elevated child taskkill cannot reach, say) cannot hang shutdown
+	// forever the way the old unconditional wait did.
+	_ = forceSignal(mp.cmd)
+	final := time.NewTimer(stopForceWait)
+	defer final.Stop()
+	select {
+	case <-mp.done:
+	case <-final.C:
+		slog.Warn("engine did not exit after a forced kill; abandoning the wait")
+	}
 }
 
 // terminatePID stops the process with the given PID (and its tree on

services/nvpair-engine-manager/proc_unix.go

@@ -29,10 +29,10 @@ func configureSysProcAttr(cmd *exec.Cmd) {
 }
 
 // gracefulSignal sends SIGTERM to the process group (falling back to the
-// process itself). It is the only stop signal engine-manager sends: stop()
-// sends this once and waits for the engine to exit, and never escalates to
-// SIGKILL. A well-behaved engine (Ollama, and the test fake, whose default
-// SIGTERM disposition is to exit) terminates on it.
+// process itself). It is the graceful stop signal: stop() sends this and
+// waits stopGrace for the engine to exit before escalating to forceSignal.
+// A well-behaved engine (Ollama, and the test fake, whose default SIGTERM
+// disposition is to exit) terminates on it.
 func gracefulSignal(cmd *exec.Cmd) error {
 	if cmd == nil || cmd.Process == nil {
 		return nil
@@ -43,6 +43,19 @@ func gracefulSignal(cmd *exec.Cmd) error {
 	return cmd.Process.Signal(syscall.SIGTERM)
 }
 
+// forceSignal sends SIGKILL to the process group (falling back to the process
+// itself). stop() escalates to it when the engine ignores the graceful signal
+// for stopGrace — SIGKILL cannot be caught, so the engine cannot survive it.
+func forceSignal(cmd *exec.Cmd) error {
+	if cmd == nil || cmd.Process == nil {
+		return nil
+	}
+	if pgid, err := syscall.Getpgid(cmd.Process.Pid); err == nil {
+		return syscall.Kill(-pgid, syscall.SIGKILL)
+	}
+	return cmd.Process.Kill()
+}
+
 // pidOnPort returns the PID listening on the given TCP port and that
 // process's executable path. Best-effort on the debug-only Unix targets: it
 // shells out to lsof, falling back to ss. ok is false when neither resolves

services/nvpair-engine-manager/proc_unix_test.go

@@ -0,0 +1,62 @@
+// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
+// SPDX-License-Identifier: Apache-2.0
+
+//go:build !windows
+
+package main
+
+import (
+	"testing"
+	"time"
+)
+
+// TestStopEscalatesOnIgnoredGracefulSignal guards the stop escalation: an engine
+// that ignores SIGTERM must still be stopped. The fake engine here traps SIGTERM
+// and keeps running, so only the forced-kill escalation after the grace can end
+// it. A stop that returns quickly with the process gone proves the escalation
+// fired; the old wait-forever behavior would hang this test past the deadline.
+func TestStopEscalatesOnIgnoredGracefulSignal(t *testing.T) {
+	// sh ignores SIGTERM (trap … ''), prints its pid, then sleeps. configureSysProcAttr
+	// puts it in its own process group, so the group kill reaches it.
+	ready := make(chan struct{}, 1)
+	proc, err := startManagedProc("sh", []string{"-c", `trap '' TERM; echo ready; sleep 60`}, nil,
+		func(_, line string) {
+			if line == "ready" {
+				select {
+				case ready <- struct{}{}:
+				default:
+				}
+			}
+		})
+	if err != nil {
+		t.Fatalf("start managed proc: %v", err)
+	}
+
+	// Wait until the trap is installed, so the graceful SIGTERM is genuinely
+	// ignored and only the escalation can stop the process.
+	select {
+	case <-ready:
+	case <-time.After(5 * time.Second):
+		t.Fatal("fake engine never signaled readiness")
+	}
+
+	rt := Runtime{Stop: &StopSpec{GraceS: 1}}
+	done := make(chan struct{})
+	go func() {
+		proc.stop(rt)
+		close(done)
+	}()
+
+	select {
+	case <-done:
+	case <-time.After(10 * time.Second):
+		t.Fatal("stop did not return: escalation never fired on an engine that ignored SIGTERM")
+	}
+
+	// The process must actually be gone, not merely abandoned.
+	select {
+	case <-proc.done:
+	case <-time.After(5 * time.Second):
+		t.Fatal("process still running after stop returned")
+	}
+}

services/nvpair-engine-manager/proc_windows.go

@@ -30,17 +30,27 @@ func configureSysProcAttr(cmd *exec.Cmd) {
 	}
 }
 
-// gracefulSignal stops the process tree and is the only stop signal
-// engine-manager sends: stop() sends this once and waits for the engine to
-// exit. Windows has no SIGTERM, and the engines we spawn run windowless
-// (CREATE_NO_WINDOW), so a non-/F taskkill only posts WM_CLOSE — which a
-// windowless process can't receive ("can only be terminated forcefully"), i.e.
-// it does nothing. Never force-killing such a process would leave the engine
-// running forever, so on Windows the stop is taskkill /T /F.
+// gracefulSignal stops the process tree with taskkill /T /F. Windows has no
+// SIGTERM, and the engines we spawn run windowless (CREATE_NO_WINDOW), so a
+// non-/F taskkill only posts WM_CLOSE — which a windowless process can't
+// receive ("can only be terminated forcefully"), i.e. it does nothing. Never
+// force-killing such a process would leave the engine running forever, so on
+// Windows the stop is immediately /T /F; stop()'s escalation timer never
+// fires because the tree is already dead by the time gracefulSignal returns.
 func gracefulSignal(cmd *exec.Cmd) error {
 	return taskkill(cmd, true)
 }
 
+// forceSignal is the escalation stop() uses when the engine ignores the
+// graceful signal. On Windows gracefulSignal already force-killed the whole
+// tree, so this is a no-op: taskkill /T /F cannot be ignored.
+func forceSignal(cmd *exec.Cmd) error {
+	if cmd == nil || cmd.Process == nil {
+		return nil
+	}
+	return taskkill(cmd, true)
+}
+
 func taskkill(cmd *exec.Cmd, force bool) error {
 	if cmd == nil || cmd.Process == nil {
 		return nil

services/nvpair-engine-manager/remediation_test.go

@@ -56,18 +56,29 @@ func capturingExecutor(t *testing.T, m *Manifest) (*Executor, *captured) {
 	return ex, c
 }
 
-// TestDownloadUnpinned verifies a fetch with no sha256 succeeds (over
-// loopback http) — the unpinned path the bundled Ollama manifest now uses.
+// TestDownloadUnpinned verifies a fetch with no sha256 is REJECTED by
+// default (fail-closed) and succeeds only with the explicit operator
+// opt-in, over loopback http.
 func TestDownloadUnpinned(t *testing.T) {
 	payload := []byte("unpinned engine bytes")
 	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		_, _ = w.Write(payload)
 	}))
 	defer srv.Close()
 	ex := newTestExecutor(t, testEngineManifest(fakeEngineBin))
+
+	// Default: fail closed, and the error must surface the computed digest
+	// so a manifest author can pin it.
+	if _, err := ex.download(context.Background(), "fake", &Fetch{URL: srv.URL}); err == nil {
+		t.Fatal("unpinned download should be rejected without the opt-in")
+	} else if !strings.Contains(err.Error(), "NVPAIR_ALLOW_UNPINNED_DOWNLOADS") {
+		t.Fatalf("rejection should name the opt-in, got %v", err)
+	}
+
+	t.Setenv("NVPAIR_ALLOW_UNPINNED_DOWNLOADS", "1")
 	p, err := ex.download(context.Background(), "fake", &Fetch{URL: srv.URL}) // no SHA256
 	if err != nil {
-		t.Fatalf("unpinned download should succeed, got %v", err)
+		t.Fatalf("unpinned download should succeed with the opt-in, got %v", err)
 	}
 	defer os.Remove(p)
 	if got, _ := os.ReadFile(p); string(got) != string(payload) {