From af2375237150b46c63ac62f934c5a2ff602e7eab Mon Sep 17 00:00:00 2001 From: woodsonl <65194841+woodsonl@users.noreply.github.com> Date: Tue, 22 Sep 2026 10:35:51 -0500 Subject: [PATCH] fix: fail closed on unpinned engine downloads and escalate engine stop Two engine-manager changes: - download() executed any HTTPS artifact whose manifest had no sha256 pin. A manifest without a digest means any host serving that URL yields code execution, so fail closed instead: return an error naming the computed digest so the author can pin it. Operators who accept unverified downloads can opt in with NVPAIR_ALLOW_UNPINNED_DOWNLOADS=1. The checksum-mismatch error names the same escape hatch, so a stale pin is not a dead end. The bundled Ollama and LM Studio fetch URLs are rolling, so their manifests stay unpinned and install through the escape hatch; the pin exists for manifests that add one. - stop() sent one graceful signal and then waited forever, so an engine that ignored SIGTERM was never stopped. It waits the manifest's stop grace (default 5s) and escalates to a forced kill of the whole process group/tree, which reaches engines that forked helper processes. Both waits are bounded, so a forced kill that itself fails cannot hang shutdown either. A test drives a process that ignores SIGTERM and asserts stop ends it after the grace. Signed-off-by: woodsonl <65194841+woodsonl@users.noreply.github.com> --- services/nvpair-engine-manager/README.md | 22 ++++--- services/nvpair-engine-manager/install.go | 29 ++++++--- services/nvpair-engine-manager/launch_test.go | 2 +- services/nvpair-engine-manager/lifecycle.go | 4 +- services/nvpair-engine-manager/proc.go | 54 ++++++++++++---- services/nvpair-engine-manager/proc_unix.go | 21 +++++-- .../nvpair-engine-manager/proc_unix_test.go | 62 +++++++++++++++++++ .../nvpair-engine-manager/proc_windows.go | 24 ++++--- .../nvpair-engine-manager/remediation_test.go | 17 ++++- 9 files changed, 191 insertions(+), 44 deletions(-) create mode 100644 services/nvpair-engine-manager/proc_unix_test.go diff --git a/services/nvpair-engine-manager/README.md b/services/nvpair-engine-manager/README.md index b401c883..3bb61370 100644 --- a/services/nvpair-engine-manager/README.md +++ b/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 diff --git a/services/nvpair-engine-manager/install.go b/services/nvpair-engine-manager/install.go index 268bd0b9..89eabf85 100644 --- a/services/nvpair-engine-manager/install.go +++ b/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. diff --git a/services/nvpair-engine-manager/launch_test.go b/services/nvpair-engine-manager/launch_test.go index d2b8eb32..488a61d0 100644 --- a/services/nvpair-engine-manager/launch_test.go +++ b/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): diff --git a/services/nvpair-engine-manager/lifecycle.go b/services/nvpair-engine-manager/lifecycle.go index cea7aa16..018d633f 100644 --- a/services/nvpair-engine-manager/lifecycle.go +++ b/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) diff --git a/services/nvpair-engine-manager/proc.go b/services/nvpair-engine-manager/proc.go index 8c92c773..4c06cce7 100644 --- a/services/nvpair-engine-manager/proc.go +++ b/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 diff --git a/services/nvpair-engine-manager/proc_unix.go b/services/nvpair-engine-manager/proc_unix.go index 876e2b69..2b58fa4b 100644 --- a/services/nvpair-engine-manager/proc_unix.go +++ b/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 diff --git a/services/nvpair-engine-manager/proc_unix_test.go b/services/nvpair-engine-manager/proc_unix_test.go new file mode 100644 index 00000000..f44ed53c --- /dev/null +++ b/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") + } +} diff --git a/services/nvpair-engine-manager/proc_windows.go b/services/nvpair-engine-manager/proc_windows.go index c383c83d..847b6486 100644 --- a/services/nvpair-engine-manager/proc_windows.go +++ b/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 diff --git a/services/nvpair-engine-manager/remediation_test.go b/services/nvpair-engine-manager/remediation_test.go index 1999373b..32052de7 100644 --- a/services/nvpair-engine-manager/remediation_test.go +++ b/services/nvpair-engine-manager/remediation_test.go @@ -56,8 +56,9 @@ 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) { @@ -65,9 +66,19 @@ func TestDownloadUnpinned(t *testing.T) { })) 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) {