From 5523e910d5e42f3e7cd63d14a1c820b8aa401d92 Mon Sep 17 00:00:00 2001 From: Jared Ping <13906915+JaredP94@users.noreply.github.com> Date: Fri, 6 Nov 2026 16:46:40 +0200 Subject: [PATCH 1/5] refactor(engine-manager): share gated Windows process jobs Extract the download job ownership and assignment gate into one helper for downloads and engine processes. Exercise assignment ordering, activation failures, termination, and idempotent handle closure through injected job operations. Signed-off-by: Jared Ping <13906915+JaredP94@users.noreply.github.com> --- .../nvpair-engine-manager/download_windows.go | 60 +++--------- services/nvpair-engine-manager/process_job.go | 57 +++++++++++ .../nvpair-engine-manager/process_job_test.go | 98 +++++++++++++++++++ .../process_job_windows.go | 90 +++++++++++++++++ 4 files changed, 256 insertions(+), 49 deletions(-) create mode 100644 services/nvpair-engine-manager/process_job.go create mode 100644 services/nvpair-engine-manager/process_job_test.go create mode 100644 services/nvpair-engine-manager/process_job_windows.go diff --git a/services/nvpair-engine-manager/download_windows.go b/services/nvpair-engine-manager/download_windows.go index d9d7750a3..d0db534d6 100644 --- a/services/nvpair-engine-manager/download_windows.go +++ b/services/nvpair-engine-manager/download_windows.go @@ -20,54 +20,24 @@ import ( ) type downloadProcess struct { - cmd *exec.Cmd - job, gate windows.Handle + cmd *exec.Cmd + owner *processJob + job windows.Handle } -func (p *downloadProcess) close() { - _ = windows.CloseHandle(p.gate) - _ = windows.CloseHandle(p.job) // KILL_ON_JOB_CLOSE also covers worker crashes. -} +func (p *downloadProcess) close() { p.owner.release() } func configureDownloadProcess(cmd *exec.Cmd) (*downloadProcess, error) { - executable, err := os.Executable() - if err != nil { - return nil, err - } - job, err := windows.CreateJobObject(nil, nil) - if err != nil { - return nil, err - } - p := &downloadProcess{cmd: cmd, job: job} - limits := windows.JOBOBJECT_EXTENDED_LIMIT_INFORMATION{} - limits.BasicLimitInformation.LimitFlags = windows.JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE - if _, err := windows.SetInformationJobObject(job, windows.JobObjectExtendedLimitInformation, uintptr(unsafe.Pointer(&limits)), uint32(unsafe.Sizeof(limits))); err != nil { - p.close() - return nil, err - } - p.gate, err = windows.CreateEvent(nil, 1, 0, nil) + owner, err := configureProcessJob(cmd, false) if err != nil { - p.close() return nil, err } - handle := p.gate - if err := windows.SetHandleInformation(handle, windows.HANDLE_FLAG_INHERIT, windows.HANDLE_FLAG_INHERIT); err != nil { - p.close() - return nil, err - } - // Keep the private-console launcher, but gate CLI creation until the - // launcher belongs to its job: no child can escape an assignment race. - cmd.Args = append([]string{executable, "--run-download", strconv.FormatUint(uint64(handle), 10), cmd.Path}, cmd.Args[1:]...) - cmd.Path = executable - cmd.SysProcAttr = &syscall.SysProcAttr{ - HideWindow: true, CreationFlags: windows.CREATE_NEW_CONSOLE, - AdditionalInheritedHandles: []syscall.Handle{syscall.Handle(handle)}, - } - return p, nil + native := owner.ops.(*windowsProcessJob) + return &downloadProcess{cmd: cmd, owner: owner, job: native.job}, nil } func killDownload(_ context.Context, process *downloadProcess) error { - return windows.TerminateJobObject(process.job, 1) + return process.owner.stop() } func terminateDownload(ctx context.Context, process *downloadProcess) error { return interruptDownload(ctx, process) @@ -106,19 +76,11 @@ func waitDownloadGroup(ctx context.Context, process *downloadProcess) error { // addresses that PID, whereas forced cleanup addresses the job. Assign the // job before opening the start gate and before cmd.Wait can reap the launcher. func pinDownloadProcess(process *downloadProcess) (func(), error) { - handle, err := windows.OpenProcess(windows.SYNCHRONIZE|windows.PROCESS_QUERY_LIMITED_INFORMATION|windows.PROCESS_SET_QUOTA|windows.PROCESS_TERMINATE, false, uint32(process.cmd.Process.Pid)) - if err != nil { - return nil, err - } - if err := windows.AssignProcessToJobObject(process.job, handle); err != nil { - _ = windows.CloseHandle(handle) - return nil, err - } - if err := windows.SetEvent(process.gate); err != nil { - _ = windows.CloseHandle(handle) + if err := process.owner.activate(); err != nil { return nil, err } - return func() { _ = windows.CloseHandle(handle) }, nil + // The shared owner pins the launcher until close, including pipe drainage. + return func() {}, nil } // runDownloadProcess owns the CLI in a private hidden console. Reset only this diff --git a/services/nvpair-engine-manager/process_job.go b/services/nvpair-engine-manager/process_job.go new file mode 100644 index 000000000..33b2bc00d --- /dev/null +++ b/services/nvpair-engine-manager/process_job.go @@ -0,0 +1,57 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +package main + +import "sync" + +// processJob owns one gated launch. The lock protects only handle operations, +// never a process wait. Implementations must not wait in these methods. +type jobOperations interface { + assign() error + resume() error + terminate() error + close() error +} + +type processJob struct { + mu sync.Mutex + ops jobOperations + closed bool +} + +func newProcessJob(ops jobOperations) *processJob { return &processJob{ops: ops} } +func (p *processJob) activate() error { + p.mu.Lock() + defer p.mu.Unlock() + if err := p.ops.assign(); err != nil { + p.abort() + return err + } + if err := p.ops.resume(); err != nil { + p.abort() + return err + } + return nil +} +func (p *processJob) abort() { + _ = p.ops.terminate() + _ = p.ops.close() + p.closed = true +} +func (p *processJob) stop() error { + p.mu.Lock() + defer p.mu.Unlock() + if p.closed { + return nil + } + return p.ops.terminate() +} +func (p *processJob) release() { + p.mu.Lock() + defer p.mu.Unlock() + if !p.closed { + _ = p.ops.close() + p.closed = true + } +} diff --git a/services/nvpair-engine-manager/process_job_test.go b/services/nvpair-engine-manager/process_job_test.go new file mode 100644 index 000000000..064e9378e --- /dev/null +++ b/services/nvpair-engine-manager/process_job_test.go @@ -0,0 +1,98 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +package main + +import ( + "errors" + "reflect" + "testing" +) + +type recordingJob struct { + events []string + fail string +} + +func (j *recordingJob) step(s string) error { + j.events = append(j.events, s) + if j.fail == s { + return errors.New(s) + } + return nil +} +func (j *recordingJob) assign() error { return j.step("assign") } +func (j *recordingJob) resume() error { return j.step("resume") } +func (j *recordingJob) terminate() error { return j.step("terminate") } +func (j *recordingJob) close() error { return j.step("close") } + +func TestProcessJobAssignmentBeforeResume(t *testing.T) { + j := &recordingJob{} + p := newProcessJob(j) + if err := p.activate(); err != nil { + t.Fatal(err) + } + if !reflect.DeepEqual(j.events, []string{"assign", "resume"}) { + t.Fatal(j.events) + } +} +func TestProcessJobActivationFailureClosesWithoutResume(t *testing.T) { + for _, fail := range []string{"assign", "resume"} { + t.Run(fail, func(t *testing.T) { + j := &recordingJob{fail: fail} + p := newProcessJob(j) + if p.activate() == nil { + t.Fatal("expected activation error") + } + want := []string{"assign", "terminate", "close"} + if fail == "resume" { + want = []string{"assign", "resume", "terminate", "close"} + } + if !reflect.DeepEqual(j.events, want) { + t.Fatal(j.events) + } + }) + } +} +func TestProcessJobStopAndCloseAreIdempotent(t *testing.T) { + j := &recordingJob{} + p := newProcessJob(j) + if err := p.activate(); err != nil { + t.Fatal(err) + } + if err := p.stop(); err != nil { + t.Fatal(err) + } + p.release() + p.release() + if err := p.stop(); err != nil { + t.Fatal(err) + } + if !reflect.DeepEqual(j.events, []string{"assign", "resume", "terminate", "close"}) { + t.Fatal(j.events) + } +} +func TestProcessJobExitAndShutdownCloseHandle(t *testing.T) { + for _, reason := range []string{"exit", "shutdown", "start failure"} { + t.Run(reason, func(t *testing.T) { + j := &recordingJob{} + p := newProcessJob(j) + p.release() + p.release() + if !reflect.DeepEqual(j.events, []string{"close"}) { + t.Fatal(j.events) + } + }) + } +} +func TestProcessJobTerminationErrorStillCloses(t *testing.T) { + j := &recordingJob{fail: "terminate"} + p := newProcessJob(j) + if p.stop() == nil { + t.Fatal("expected termination error") + } + p.release() + if !reflect.DeepEqual(j.events, []string{"terminate", "close"}) { + t.Fatal(j.events) + } +} diff --git a/services/nvpair-engine-manager/process_job_windows.go b/services/nvpair-engine-manager/process_job_windows.go new file mode 100644 index 000000000..6feb3d962 --- /dev/null +++ b/services/nvpair-engine-manager/process_job_windows.go @@ -0,0 +1,90 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +//go:build windows + +package main + +import ( + "fmt" + "os" + "os/exec" + "strconv" + "syscall" + "unsafe" + + "golang.org/x/sys/windows" +) + +// windowsProcessJob gates an engine-manager launcher before it can spawn the +// target. Only the event is inherited; the kill-on-close job and pinned process +// handle remain exclusively in the manager. Both downloads and engines use it. +type windowsProcessJob struct { + cmd *exec.Cmd + job, gate, process windows.Handle +} + +func configureProcessJob(cmd *exec.Cmd, breakaway bool) (*processJob, error) { + executable, err := os.Executable() + if err != nil { + return nil, err + } + job, err := windows.CreateJobObject(nil, nil) + if err != nil { + return nil, err + } + native := &windowsProcessJob{cmd: cmd, job: job} + owner := newProcessJob(native) + limits := windows.JOBOBJECT_EXTENDED_LIMIT_INFORMATION{} + limits.BasicLimitInformation.LimitFlags = windows.JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE + // Command-mode vendor CLIs can explicitly launch an independent daemon with + // CREATE_BREAKAWAY_FROM_JOB. Never enable SILENT_BREAKAWAY: ordinary children + // must remain owned, even when they reparent or detach their console. + if breakaway { + limits.BasicLimitInformation.LimitFlags |= windows.JOB_OBJECT_LIMIT_BREAKAWAY_OK + } + if _, err := windows.SetInformationJobObject(job, windows.JobObjectExtendedLimitInformation, uintptr(unsafe.Pointer(&limits)), uint32(unsafe.Sizeof(limits))); err != nil { + owner.release() + return nil, err + } + native.gate, err = windows.CreateEvent(nil, 1, 0, nil) + if err != nil { + owner.release() + return nil, err + } + if err := windows.SetHandleInformation(native.gate, windows.HANDLE_FLAG_INHERIT, windows.HANDLE_FLAG_INHERIT); err != nil { + owner.release() + return nil, err + } + cmd.Args = append([]string{executable, "--run-download", strconv.FormatUint(uint64(native.gate), 10), cmd.Path}, cmd.Args[1:]...) + cmd.Path = executable + cmd.SysProcAttr = &syscall.SysProcAttr{HideWindow: true, CreationFlags: windows.CREATE_NEW_CONSOLE, AdditionalInheritedHandles: []syscall.Handle{syscall.Handle(native.gate)}} + return owner, nil +} + +func (p *windowsProcessJob) assign() error { + // cmd.Wait cannot run before activation, so os.Process still holds the + // original process object and its PID cannot have been recycled. + handle, err := windows.OpenProcess(windows.SYNCHRONIZE|windows.PROCESS_QUERY_LIMITED_INFORMATION|windows.PROCESS_SET_QUOTA|windows.PROCESS_TERMINATE, false, uint32(p.cmd.Process.Pid)) + if err != nil { + return err + } + p.process = handle + if err := windows.AssignProcessToJobObject(p.job, handle); err != nil { + // The gated launcher has no children yet and is not in our job. Kill only + // the pinned process object, never a newly looked-up PID. + _ = windows.TerminateProcess(handle, 1) + return fmt.Errorf("assign process job: %w", err) + } + return nil +} +func (p *windowsProcessJob) resume() error { return windows.SetEvent(p.gate) } +func (p *windowsProcessJob) terminate() error { return windows.TerminateJobObject(p.job, 1) } +func (p *windowsProcessJob) close() error { + for _, h := range []windows.Handle{p.gate, p.process} { + if h != 0 { + _ = windows.CloseHandle(h) + } + } + return windows.CloseHandle(p.job) +} From 113eed2095263ac9c56809bc0bcbda77bd5848a1 Mon Sep 17 00:00:00 2001 From: Jared Ping <13906915+JaredP94@users.noreply.github.com> Date: Fri, 6 Nov 2026 17:12:12 +0200 Subject: [PATCH 2/5] fix(engine-manager): own Windows engine processes with jobs Assign servers, action CLIs, and install helpers to kill-on-close jobs before releasing their start gates. Use the same ownership helper as downloads and terminate jobs instead of taskkill PID trees. Keep command-mode vendor daemons independent by assigning their suspended control CLIs to CLI-only jobs, preserving bounded official stop commands. Bound command execution and output drainage, close jobs on exit and shutdown, and pin and revalidate legacy orphan handles before reclaiming them. Preserve Unix process behavior. Document the ownership boundary and add injected-job lifecycle and error-path regression tests. Windows runtime behavior remains untested. Signed-off-by: Jared Ping <13906915+JaredP94@users.noreply.github.com> --- services/nvpair-engine-manager/README.md | 40 ++++- services/nvpair-engine-manager/actions.go | 2 +- .../nvpair-engine-manager/command_context.go | 18 +++ .../nvpair-engine-manager/command_process.go | 30 ++++ .../command_process_unix.go | 13 ++ .../command_process_windows.go | 25 ++++ .../nvpair-engine-manager/download_windows.go | 2 +- .../engine_process_unix.go | 41 ++++++ .../engine_process_windows.go | 69 +++++++++ services/nvpair-engine-manager/install.go | 4 +- services/nvpair-engine-manager/job_command.go | 31 ++++ .../nvpair-engine-manager/job_command_test.go | 137 ++++++++++++++++++ services/nvpair-engine-manager/lifecycle.go | 7 +- .../loadprofiles_actions.go | 4 +- services/nvpair-engine-manager/proc.go | 61 ++------ .../nvpair-engine-manager/proc_windows.go | 83 ++++------- services/nvpair-engine-manager/process_job.go | 26 +++- .../nvpair-engine-manager/process_job_test.go | 19 ++- .../process_job_windows.go | 49 ++++++- .../testprocess_windows_test.go | 4 +- 20 files changed, 536 insertions(+), 129 deletions(-) create mode 100644 services/nvpair-engine-manager/command_context.go create mode 100644 services/nvpair-engine-manager/command_process.go create mode 100644 services/nvpair-engine-manager/command_process_unix.go create mode 100644 services/nvpair-engine-manager/command_process_windows.go create mode 100644 services/nvpair-engine-manager/engine_process_unix.go create mode 100644 services/nvpair-engine-manager/engine_process_windows.go create mode 100644 services/nvpair-engine-manager/job_command.go create mode 100644 services/nvpair-engine-manager/job_command_test.go diff --git a/services/nvpair-engine-manager/README.md b/services/nvpair-engine-manager/README.md index 207b1467b..07ff26e87 100644 --- a/services/nvpair-engine-manager/README.md +++ b/services/nvpair-engine-manager/README.md @@ -580,7 +580,7 @@ numbers and never add the two together: they count overlapping work. ``` NotInstalled --engine:install--> (HTTPS download + verify-if-pinned + 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-----> (platform stop / bounded official stop CLI) --> Stopped ``` Detect uses the manifest's executable candidates (`runtime.cli` when explicitly @@ -592,11 +592,10 @@ 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. +reports the failed start. Process-mode Stop preserves SIGTERM to the process +group on Unix (graceful, no SIGKILL escalation or exit timeout). On Windows it +terminates the owned job, closes its kill-on-close handle, and bounds the final +exit wait to five seconds. There is no PID-addressed tree kill. Command-mode stop (including oMLX) gives the official stop CLI `max(grace_s, 5s)` to return, then up to `grace_s` (default 5s) for shutdown. @@ -609,6 +608,35 @@ held across these waits; lifecycle operations remain serialized so an immediate start cannot overtake stop. A timeout leaves the engine running, not falsely stopped, and is returned to the caller. +### Windows process ownership + +One shared kill-on-close job helper is used by downloads and all Windows +processes spawned by engine-manager: + +| Launch | Job membership and lifetime | +| --- | --- | +| Process-mode engine server | Gated hidden-console launcher, server, and all descendants; no breakaway. Stop terminates the job; launcher exit also closes the job and kills leftovers. | +| Model download | Same launcher and full descendant job, preserving the existing bounded console-interrupt/retry/kill sequence. | +| Action CLIs, install/uninstall helpers, console-interrupt helper | One job per invocation, including launcher and descendants; no breakaway. Context cancellation terminates the job; completion closes it. Commands have a 15-minute safety ceiling in addition to caller deadlines, with one-second inherited-output drainage. | +| Command-mode start/stop CLIs (`lms server start/stop`, `omlx stop` where supported) | The actual CLI starts suspended and is assigned before its initial thread resumes. Its job owns **only the control CLI**; silent child breakaway deliberately preserves the command-mode contract that the vendor owns its daemon tree. These children are not reclaimed by job termination or by PID. The existing bounded official stop command and shutdown confirmation remain authoritative. | + +Only command-mode lifecycle controls get that child-breakaway exception; normal +CLIs and install/uninstall helpers do not. All jobs and process handles remain +non-inheritable. The gated launcher inherits only its start event, which it +closes before launching the target. Assignment/resume failures terminate the +owned process and close the job; cancellation cannot resume an unassigned or +already-cancelled launch. No state mutex or job-handle mutex covers a process +wait. Existing lifecycle-operation serialization remains unchanged. + +No `taskkill` calls remain. Legacy adopted Windows orphans have no job: reclaim +reopens and pins the process, revalidates its image against the already-verified +PAIR-owned install, then terminates **that handle only**. It does not enumerate +or kill descendants for which ownership cannot be proved. Foreign listeners and +vendor-managed daemon trees remain untouched. + +Windows runtime behavior is untested here; cross-compilation and injected-job +unit tests do not establish runtime behavior on Windows. + ### Adoption — start may attach to an engine it didn't launch Before spawning, `engine:start` **probes the chosen port's readiness diff --git a/services/nvpair-engine-manager/actions.go b/services/nvpair-engine-manager/actions.go index 2927e85dc..d127c7a4c 100644 --- a/services/nvpair-engine-manager/actions.go +++ b/services/nvpair-engine-manager/actions.go @@ -321,7 +321,7 @@ func (e *Executor) runCommandOutput(ctx context.Context, argv []string) (string, } cmd := exec.CommandContext(ctx, argv[0], argv[1:]...) configureSysProcAttr(cmd) - out, err := cmd.Output() + out, err := ownedOutput(cmd) if err != nil { if ee, ok := err.(*exec.ExitError); ok { return "", fmt.Errorf("%v: %s", err, strings.TrimSpace(string(ee.Stderr))) diff --git a/services/nvpair-engine-manager/command_context.go b/services/nvpair-engine-manager/command_context.go new file mode 100644 index 000000000..0f294a41b --- /dev/null +++ b/services/nvpair-engine-manager/command_context.go @@ -0,0 +1,18 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +package main + +import "context" + +type controlCommandKey struct{} + +// Only command-mode start/stop declares an independently managed daemon tree. +// Install/uninstall helpers and action CLIs retain normal tree ownership. +func withControlCommand(ctx context.Context) context.Context { + return context.WithValue(ctx, controlCommandKey{}, true) +} +func isControlCommand(ctx context.Context) bool { + control, _ := ctx.Value(controlCommandKey{}).(bool) + return control +} diff --git a/services/nvpair-engine-manager/command_process.go b/services/nvpair-engine-manager/command_process.go new file mode 100644 index 000000000..8cde578cf --- /dev/null +++ b/services/nvpair-engine-manager/command_process.go @@ -0,0 +1,30 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +//go:build windows + +package main + +import ( + "bytes" + "errors" + "os/exec" +) + +func ownedCombinedOutput(cmd *exec.Cmd, detachedControl bool) ([]byte, error) { + var output bytes.Buffer + cmd.Stdout, cmd.Stderr = &output, &output + err := runOwnedCommandMode(cmd, detachedControl) + return output.Bytes(), err +} + +func ownedOutput(cmd *exec.Cmd) ([]byte, error) { + var output, diagnostic bytes.Buffer + cmd.Stdout, cmd.Stderr = &output, &diagnostic + err := runOwnedCommand(cmd) + var exit *exec.ExitError + if errors.As(err, &exit) { + exit.Stderr = diagnostic.Bytes() + } + return output.Bytes(), err +} diff --git a/services/nvpair-engine-manager/command_process_unix.go b/services/nvpair-engine-manager/command_process_unix.go new file mode 100644 index 000000000..f3a126d67 --- /dev/null +++ b/services/nvpair-engine-manager/command_process_unix.go @@ -0,0 +1,13 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +//go:build !windows + +package main + +import "os/exec" + +func ownedCombinedOutput(cmd *exec.Cmd, _ bool) ([]byte, error) { return cmd.CombinedOutput() } +func ownedOutput(cmd *exec.Cmd) ([]byte, error) { return cmd.Output() } +func runOwnedCommandMode(cmd *exec.Cmd, _ bool) error { return cmd.Run() } +func runOwnedCommand(cmd *exec.Cmd) error { return cmd.Run() } diff --git a/services/nvpair-engine-manager/command_process_windows.go b/services/nvpair-engine-manager/command_process_windows.go new file mode 100644 index 000000000..ba0a5f391 --- /dev/null +++ b/services/nvpair-engine-manager/command_process_windows.go @@ -0,0 +1,25 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +//go:build windows + +package main + +import ( + "os/exec" + "time" +) + +// Even a caller without a context deadline cannot wait forever for a helper. +// More restrictive caller deadlines still cancel the entire owned job. +const windowsCommandLimit = 15 * time.Minute + +func runOwnedCommand(cmd *exec.Cmd) error { return runOwnedCommandMode(cmd, false) } + +func runOwnedCommandMode(cmd *exec.Cmd, detachedControl bool) error { + owner, err := configureProcessJob(cmd, detachedControl) + if err != nil { + return err + } + return runJobCommand(cmd, owner, windowsCommandLimit) +} diff --git a/services/nvpair-engine-manager/download_windows.go b/services/nvpair-engine-manager/download_windows.go index d0db534d6..01078be3a 100644 --- a/services/nvpair-engine-manager/download_windows.go +++ b/services/nvpair-engine-manager/download_windows.go @@ -165,7 +165,7 @@ func interruptDownload(parent context.Context, process *downloadProcess) error { defer cancel() helper := exec.CommandContext(ctx, executable, "--interrupt-download", strconv.Itoa(cmd.Process.Pid)) configureSysProcAttr(helper) - return helper.Run() + return runOwnedCommand(helper) } // Download launch and console attachment run in helpers so engine-manager's diff --git a/services/nvpair-engine-manager/engine_process_unix.go b/services/nvpair-engine-manager/engine_process_unix.go new file mode 100644 index 000000000..3820a02c3 --- /dev/null +++ b/services/nvpair-engine-manager/engine_process_unix.go @@ -0,0 +1,41 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +//go:build !windows + +package main + +import ( + "io" + "os/exec" + "sync" + "time" +) + +// Preserve Unix orphan reclaim and its existing signal/deadline semantics. +func terminateOwnedPID(pid int, grace time.Duration, _ string) { + if pid <= 0 { + return + } + _ = signalPID(pid, false) + if grace <= 0 { + grace = 5 * time.Second + } + deadline := time.Now().Add(grace) + for time.Now().Before(deadline) { + if !pidAlive(pid) { + return + } + time.Sleep(100 * time.Millisecond) + } + if pidAlive(pid) { + _ = signalPID(pid, true) + } +} + +func startEngineCommand(cmd *exec.Cmd) (*processJob, error) { return nil, cmd.Start() } +func waitEngineCommand(mp *managedProc, scans *sync.WaitGroup, _, _ io.Closer) { + scans.Wait() + _ = mp.cmd.Wait() +} +func stopEngineCommand(mp *managedProc) { _ = gracefulSignal(mp.cmd); <-mp.done } diff --git a/services/nvpair-engine-manager/engine_process_windows.go b/services/nvpair-engine-manager/engine_process_windows.go new file mode 100644 index 000000000..e93e3d07d --- /dev/null +++ b/services/nvpair-engine-manager/engine_process_windows.go @@ -0,0 +1,69 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +//go:build windows + +package main + +import ( + "io" + "os/exec" + "sync" + "time" + + "golang.org/x/sys/windows" +) + +func startEngineCommand(cmd *exec.Cmd) (*processJob, error) { + owner, err := configureProcessJob(cmd, false) + if err != nil { + return nil, err + } + if err := cmd.Start(); err != nil { + owner.release() + return nil, err + } + if err := owner.activate(); err != nil { + _ = cmd.Process.Kill() // The original os.Process handle still proves identity. + cmd.WaitDelay = time.Second + _ = cmd.Wait() + return nil, err + } + return owner, nil +} + +func waitEngineCommand(mp *managedProc, scans *sync.WaitGroup, stdout, stderr io.Closer) { + handle, openErr := windows.OpenProcess(windows.SYNCHRONIZE, false, uint32(mp.cmd.Process.Pid)) + if openErr == nil { + defer windows.CloseHandle(handle) + } + // Observe exit without releasing cmd's process handle or closing its pipes. + // Polling waits are bounded; Stop terminates the job to end engine lifetime. + for openErr == nil { + status, err := windows.WaitForSingleObject(handle, 1000) + if err != nil || status != uint32(windows.WAIT_TIMEOUT) { + break + } + } + mp.owner.release() // Also kill descendants that outlived the launcher. + drained := make(chan struct{}) + go func() { scans.Wait(); close(drained) }() + select { + case <-drained: + case <-time.After(time.Second): + _ = stdout.Close() + _ = stderr.Close() + } + mp.cmd.WaitDelay = time.Second + _ = mp.cmd.Wait() +} + +func stopEngineCommand(mp *managedProc) { + _ = mp.owner.stop() + // Kill-on-close is the final fallback, also on errors from TerminateJobObject. + mp.owner.release() + select { + case <-mp.done: + case <-time.After(5 * time.Second): + } +} diff --git a/services/nvpair-engine-manager/install.go b/services/nvpair-engine-manager/install.go index 543e13d74..99a9a288e 100644 --- a/services/nvpair-engine-manager/install.go +++ b/services/nvpair-engine-manager/install.go @@ -412,7 +412,7 @@ func (e *Executor) runCommand(ctx context.Context, argv []string) error { } cmd := exec.CommandContext(ctx, argv[0], argv[1:]...) configureSysProcAttr(cmd) // hide the console window on Windows - out, err := cmd.CombinedOutput() + out, err := ownedCombinedOutput(cmd, isControlCommand(ctx)) if err != nil { return fmt.Errorf("%w: %s", err, strings.TrimSpace(string(out))) } @@ -453,7 +453,7 @@ func (e *Executor) runCommandTail(ctx context.Context, argv []string, maxLines, tail := &commandTail{maxBytes: maxBytes} cmd.Stdout = io.Discard cmd.Stderr = tail - if err := cmd.Run(); err != nil { + if err := runOwnedCommand(cmd); err != nil { lines := strings.SplitAfter(string(tail.data), "\n") if lines[len(lines)-1] == "" { lines = lines[:len(lines)-1] diff --git a/services/nvpair-engine-manager/job_command.go b/services/nvpair-engine-manager/job_command.go new file mode 100644 index 000000000..636ae5e5e --- /dev/null +++ b/services/nvpair-engine-manager/job_command.go @@ -0,0 +1,31 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +package main + +import ( + "os/exec" + "time" +) + +// runJobCommand is the platform-independent ownership lifetime. Activation is +// before Wait (and before the gated child can execute); every exit path releases +// the job. The owner serializes handle operations only, never Start or Wait. +func runJobCommand(cmd *exec.Cmd, owner *processJob, limit time.Duration) error { + defer owner.release() + if cmd.Cancel != nil { + cmd.Cancel = owner.stop + } + cmd.WaitDelay = time.Second + if err := cmd.Start(); err != nil { + return err + } + if err := owner.activate(); err != nil { + _ = cmd.Process.Kill() // Original process handle, never a PID lookup. + _ = cmd.Wait() + return err + } + timer := time.AfterFunc(limit, func() { _ = owner.stop() }) + defer timer.Stop() + return cmd.Wait() +} diff --git a/services/nvpair-engine-manager/job_command_test.go b/services/nvpair-engine-manager/job_command_test.go new file mode 100644 index 000000000..3a8a00131 --- /dev/null +++ b/services/nvpair-engine-manager/job_command_test.go @@ -0,0 +1,137 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +package main + +import ( + "context" + "os" + "os/exec" + "sync" + "testing" + "time" +) + +// Test the shared command lifetime, not Windows syscalls, with a real child +// process and injected ownership operations. No engine installation is involved. +type commandJob struct { + recordingJob + cmd *exec.Cmd + activated chan struct{} + once sync.Once +} + +func (j *commandJob) resume() error { + j.once.Do(func() { close(j.activated) }) + return j.recordingJob.resume() +} +func (j *commandJob) terminate() error { + err := j.recordingJob.terminate() + if j.cmd.Process != nil { + _ = j.cmd.Process.Kill() + } + return err +} + +func TestJobCommandFixture(t *testing.T) { + if os.Getenv("PAIR_JOB_FIXTURE") != "1" { + return + } + if os.Getenv("PAIR_JOB_SLEEP") == "1" { + time.Sleep(30 * time.Second) + } + os.Exit(0) +} + +func jobFixture(ctx context.Context, sleep bool) *exec.Cmd { + cmd := exec.CommandContext(ctx, os.Args[0], "-test.run=^TestJobCommandFixture$") + cmd.Env = append(os.Environ(), "PAIR_JOB_FIXTURE=1") + if sleep { + cmd.Env = append(cmd.Env, "PAIR_JOB_SLEEP=1") + } + return cmd +} + +func TestJobCommandExitClosesJob(t *testing.T) { + cmd := jobFixture(context.Background(), false) + j := &commandJob{cmd: cmd, activated: make(chan struct{})} + // Race-instrumented helper exit includes the runtime's one-second drain. + if err := runJobCommand(cmd, newProcessJob(j), 10*time.Second); err != nil { + t.Fatal(err) + } + if got := j.events[len(j.events)-1]; got != "close" { + t.Fatal(j.events) + } +} +func TestJobCommandStartFailureClosesJob(t *testing.T) { + cmd := exec.Command("/nonexistent/pair-job-command") + j := &commandJob{cmd: cmd, activated: make(chan struct{})} + if runJobCommand(cmd, newProcessJob(j), time.Second) == nil { + t.Fatal("expected start failure") + } + if len(j.events) != 1 || j.events[0] != "close" { + t.Fatal(j.events) + } +} +func TestJobCommandAssignmentFailureReapsAndCloses(t *testing.T) { + cmd := jobFixture(context.Background(), true) + j := &commandJob{recordingJob: recordingJob{fail: "assign"}, cmd: cmd, activated: make(chan struct{})} + if runJobCommand(cmd, newProcessJob(j), time.Second) == nil { + t.Fatal("expected assignment failure") + } + if cmd.ProcessState == nil { + t.Fatal("child not reaped") + } + if j.events[len(j.events)-1] != "close" { + t.Fatal(j.events) + } +} +func TestJobCommandShutdownTerminatesAndClosesJob(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + cmd := jobFixture(ctx, true) + j := &commandJob{cmd: cmd, activated: make(chan struct{})} + done := make(chan error, 1) + go func() { done <- runJobCommand(cmd, newProcessJob(j), time.Second) }() + select { + case <-j.activated: + case <-time.After(5 * time.Second): + t.Fatal("command not activated") + } + cancel() + select { + case err := <-done: + if err == nil { + t.Fatal("expected cancellation") + } + case <-time.After(5 * time.Second): + t.Fatal("shutdown blocked") + } + if j.events[len(j.events)-2] != "terminate" || j.events[len(j.events)-1] != "close" { + t.Fatal(j.events) + } +} +func TestJobCommandDeadlineTerminatesJob(t *testing.T) { + cmd := jobFixture(context.Background(), true) + j := &commandJob{cmd: cmd, activated: make(chan struct{})} + if runJobCommand(cmd, newProcessJob(j), 10*time.Millisecond) == nil { + t.Fatal("expected deadline kill") + } + if j.events[len(j.events)-2] != "terminate" || j.events[len(j.events)-1] != "close" { + t.Fatal(j.events) + } +} +func TestControlCommandOwnershipIsExplicit(t *testing.T) { + ctx := context.Background() + if isControlCommand(ctx) { + t.Fatal("ordinary commands must own descendants") + } + marked, cancel := context.WithTimeout(withControlCommand(ctx), time.Second) + defer cancel() + if !isControlCommand(marked) { + t.Fatal("lost command-mode ownership across deadline") + } + if isControlCommand(ctx) { + t.Fatal("control marker leaked to other helpers") + } +} diff --git a/services/nvpair-engine-manager/lifecycle.go b/services/nvpair-engine-manager/lifecycle.go index ca58de9df..e8a5375dd 100644 --- a/services/nvpair-engine-manager/lifecycle.go +++ b/services/nvpair-engine-manager/lifecycle.go @@ -317,6 +317,7 @@ func (e *Executor) bringUpProcess(ctx context.Context, st *engineState, engine s // daemon-style engine such as LM Studio), then waits for readiness. // There is no owned process — liveness comes from the probe. func (e *Executor) bringUpCommand(ctx context.Context, st *engineState, engine string, rt Runtime, port int, vars map[string]string) error { + ctx = withControlCommand(ctx) st.mu.Lock() st.proc = nil st.mu.Unlock() @@ -398,7 +399,7 @@ func runPrivateLaunchCommand(ctx context.Context, argv []string, environment map } } configureSysProcAttr(cmd) - if err := cmd.Run(); err != nil { + if err := runOwnedCommandMode(cmd, isControlCommand(ctx)); err != nil { failure := fmt.Errorf("launch command failed: %w", err) if len(privateArgs) > 0 || len(environment) > 0 { return failure @@ -497,7 +498,7 @@ func (e *Executor) doStop(st *engineState, engine string) error { } st.mu.Unlock() grace := stopGrace(rt) - terminatePID(pid, grace) + terminateOwnedPID(pid, grace, binPath) if !pidAlive(pid) { e.markStopped(st, engine) return nil @@ -576,7 +577,7 @@ func (e *Executor) runCommandStop(st *engineState, engine string, rt Runtime, po pid = 0 } } - stopCtx, cancelStop := context.WithTimeout(context.Background(), commandStopTimeout(rt)) + stopCtx, cancelStop := context.WithTimeout(withControlCommand(context.Background()), commandStopTimeout(rt)) runErr := e.runCommand(stopCtx, argv) cancelStop() if rt.Ready != nil { diff --git a/services/nvpair-engine-manager/loadprofiles_actions.go b/services/nvpair-engine-manager/loadprofiles_actions.go index cb3280be3..21c2f3b5c 100644 --- a/services/nvpair-engine-manager/loadprofiles_actions.go +++ b/services/nvpair-engine-manager/loadprofiles_actions.go @@ -47,7 +47,7 @@ func (e *Executor) listLMSInstances(ctx context.Context, cli string) (map[string var output lmsPSOutput command.Stdout = &output // stderr is discarded; stdout is bounded private structured data, never logs. - if err := command.Run(); err != nil || ctx.Err() != nil { + if err := runOwnedCommand(command); err != nil || ctx.Err() != nil { return nil, errors.New(profiledListFailed) } var instances []struct { @@ -84,7 +84,7 @@ func runPrivateLMSCommand(ctx context.Context, argv []string) error { command := exec.CommandContext(ctx, argv[0], argv[1:]...) configureSysProcAttr(command) command.WaitDelay = time.Second - if err := command.Run(); err != nil || ctx.Err() != nil { + if err := runOwnedCommand(command); err != nil || ctx.Err() != nil { return errors.New(profiledLoadFailed) } return nil diff --git a/services/nvpair-engine-manager/proc.go b/services/nvpair-engine-manager/proc.go index 07b4d4272..ca0025dbc 100644 --- a/services/nvpair-engine-manager/proc.go +++ b/services/nvpair-engine-manager/proc.go @@ -11,7 +11,6 @@ import ( "path/filepath" "strings" "sync" - "time" ) // managedProc is a spawned engine process with its stdout/stderr @@ -20,8 +19,9 @@ import ( // graceful signal, force kill) live in proc_windows.go / proc_unix.go // so the same logic here runs on every platform. type managedProc struct { - cmd *exec.Cmd - done chan struct{} + cmd *exec.Cmd + done chan struct{} + owner *processJob // Windows only; Unix retains its existing process groups. } // startManagedProc launches bin with args and the extra env (merged @@ -46,11 +46,14 @@ func startManagedProc(bin string, args []string, env map[string]string, onLine f if err != nil { return nil, err } - if err := cmd.Start(); err != nil { + owner, err := startEngineCommand(cmd) + if err != nil { + _ = stdout.Close() + _ = stderr.Close() return nil, err } - mp := &managedProc{cmd: cmd, done: make(chan struct{})} + mp := &managedProc{cmd: cmd, done: make(chan struct{}), owner: owner} var scanWg sync.WaitGroup scanWg.Add(2) go func() { defer scanWg.Done(); scanLines(stdout, "stdout", onLine) }() @@ -58,8 +61,7 @@ func startManagedProc(bin string, args []string, env map[string]string, onLine f go func() { // Drain stdout/stderr fully before Wait: Wait closes the pipes on // exit, which would otherwise truncate the final captured lines. - scanWg.Wait() - _ = cmd.Wait() + waitEngineCommand(mp, &scanWg, stdout, stderr) close(mp.done) }() return mp, nil @@ -75,20 +77,17 @@ 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 using the platform's ownership-aware stop path. // // 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. -// - 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. +// - Windows: terminate the owned job; no PID lookup or tree enumeration. +// Command-mode engines use their bounded official stop CLI separately. // -// 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. +// Unix deliberately retains its existing unbounded SIGTERM wait. Windows +// bounds its final exit/drain wait after terminating and closing the owned job. func (mp *managedProc) stop() { if mp == nil || mp.cmd == nil || mp.cmd.Process == nil { return @@ -98,37 +97,7 @@ func (mp *managedProc) stop() { return // already exited default: } - _ = gracefulSignal(mp.cmd) - <-mp.done -} - -// terminatePID stops the process with the given PID (and its tree on -// Windows, or its process group on Unix when available): a graceful signal -// first, escalating to a forced kill if the process hasn't exited within -// grace. It exists to reclaim a PAIR-managed engine orphan adopted on our -// own port — an instance a prior run spawned and then lost the handle to, so -// we can only address it by PID rather than through the *exec.Cmd handle -// managedProc.stop needs. Best-effort: a process that's already gone counts -// as success. The platform primitives (signalPID, pidAlive) live in -// proc_windows.go / proc_unix.go. -func terminatePID(pid int, grace time.Duration) { - if pid <= 0 { - return - } - _ = signalPID(pid, false) - if grace <= 0 { - grace = 5 * time.Second - } - deadline := time.Now().Add(grace) - for time.Now().Before(deadline) { - if !pidAlive(pid) { - return - } - time.Sleep(100 * time.Millisecond) - } - if pidAlive(pid) { - _ = signalPID(pid, true) - } + stopEngineCommand(mp) } // normalizeEngineImage cleans an executable path for comparison. Linux diff --git a/services/nvpair-engine-manager/proc_windows.go b/services/nvpair-engine-manager/proc_windows.go index 97059970a..ac9472ba8 100644 --- a/services/nvpair-engine-manager/proc_windows.go +++ b/services/nvpair-engine-manager/proc_windows.go @@ -6,9 +6,7 @@ package main import ( - "context" "os/exec" - "strconv" "syscall" "time" "unsafe" @@ -16,11 +14,6 @@ import ( "golang.org/x/sys/windows" ) -// taskkillTimeout bounds each taskkill invocation so a hung taskkill can't -// wedge StopAll (and the whole app shutdown) now that the broker no longer -// force-kills engine-manager on a timeout. -const taskkillTimeout = 5 * time.Second - // configureSysProcAttr hides the child's console window // (HideWindow + CREATE_NO_WINDOW), matching every other NVPAIR subprocess. func configureSysProcAttr(cmd *exec.Cmd) { @@ -30,36 +23,6 @@ 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. -func gracefulSignal(cmd *exec.Cmd) error { - return taskkill(cmd, true) -} - -func taskkill(cmd *exec.Cmd, force bool) error { - return taskkillContext(context.Background(), cmd, force) -} - -func taskkillContext(parent context.Context, cmd *exec.Cmd, force bool) error { - if cmd == nil || cmd.Process == nil { - return nil - } - args := []string{"/T", "/PID", strconv.Itoa(cmd.Process.Pid)} - if force { - args = append([]string{"/F"}, args...) - } - ctx, cancel := context.WithTimeout(parent, taskkillTimeout) - defer cancel() - kc := exec.CommandContext(ctx, "taskkill", args...) - configureSysProcAttr(kc) // hide taskkill's own console window - return kc.Run() -} - var ( iphlpapi = windows.NewLazySystemDLL("iphlpapi.dll") procGetExtendedTcpTable = iphlpapi.NewProc("GetExtendedTcpTable") @@ -176,23 +139,39 @@ func imagePathForPID(pid int) string { return windows.UTF16ToString(buf[:size]) } -// signalPID asks the PID's process tree to stop (taskkill /T), escalating to -// a forced /F kill when force is set. It is the PID-addressed kill used only by -// the orphan reclaim (a process we lost the *exec.Cmd handle to), distinct from -// the normal graceful-only stop() path. -func signalPID(pid int, force bool) error { - if pid <= 0 { - return nil +// A legacy orphan has no job. Revalidate the expected PAIR-installed image +// through a pinned handle and terminate only that process object. Enumerating +// and killing a PID tree cannot establish ownership of its current children. +// The caller has already checked that expectedImage is in an owned install. +func terminateOwnedPID(pid int, grace time.Duration, expectedImage string) { + if pid <= 0 || expectedImage == "" { + return + } + h, err := windows.OpenProcess(windows.SYNCHRONIZE|windows.PROCESS_QUERY_LIMITED_INFORMATION|windows.PROCESS_TERMINATE, false, uint32(pid)) + if err != nil { + return } - args := []string{"/T", "/PID", strconv.Itoa(pid)} - if force { - args = append([]string{"/F"}, args...) + defer windows.CloseHandle(h) + buf := make([]uint16, 32768) + size := uint32(len(buf)) + if windows.QueryFullProcessImageName(h, 0, &buf[0], &size) != nil || !isOurEngineImage(windows.UTF16ToString(buf[:size]), expectedImage) { + return + } + // Windowless legacy engines cannot receive WM_CLOSE. Unlike owned jobs, + // there is no safe way to reclaim descendants without their own proof. + if windows.TerminateProcess(h, 1) != nil { + return + } + if grace <= 0 { + grace = 5 * time.Second + } + deadline := time.Now().Add(grace) + for time.Now().Before(deadline) { + status, err := windows.WaitForSingleObject(h, 100) + if err != nil || status != uint32(windows.WAIT_TIMEOUT) { + return + } } - ctx, cancel := context.WithTimeout(context.Background(), taskkillTimeout) - defer cancel() - kc := exec.CommandContext(ctx, "taskkill", args...) - configureSysProcAttr(kc) // hide taskkill's own console window - return kc.Run() } // pidAlive reports whether the PID is still running (WaitForSingleObject with diff --git a/services/nvpair-engine-manager/process_job.go b/services/nvpair-engine-manager/process_job.go index 33b2bc00d..6ce97a62e 100644 --- a/services/nvpair-engine-manager/process_job.go +++ b/services/nvpair-engine-manager/process_job.go @@ -3,7 +3,10 @@ package main -import "sync" +import ( + "os" + "sync" +) // processJob owns one gated launch. The lock protects only handle operations, // never a process wait. Implementations must not wait in these methods. @@ -15,15 +18,19 @@ type jobOperations interface { } type processJob struct { - mu sync.Mutex - ops jobOperations - closed bool + mu sync.Mutex + ops jobOperations + closed bool + stopped bool } func newProcessJob(ops jobOperations) *processJob { return &processJob{ops: ops} } func (p *processJob) activate() error { p.mu.Lock() defer p.mu.Unlock() + if p.closed || p.stopped { + return os.ErrProcessDone + } if err := p.ops.assign(); err != nil { p.abort() return err @@ -42,10 +49,17 @@ func (p *processJob) abort() { func (p *processJob) stop() error { p.mu.Lock() defer p.mu.Unlock() - if p.closed { + if p.closed || p.stopped { return nil } - return p.ops.terminate() + p.stopped = true + if err := p.ops.terminate(); err != nil { + // Kill-on-close is the fallback even if explicit termination fails. + _ = p.ops.close() + p.closed = true + return err + } + return nil } func (p *processJob) release() { p.mu.Lock() diff --git a/services/nvpair-engine-manager/process_job_test.go b/services/nvpair-engine-manager/process_job_test.go index 064e9378e..c439c5f09 100644 --- a/services/nvpair-engine-manager/process_job_test.go +++ b/services/nvpair-engine-manager/process_job_test.go @@ -26,6 +26,21 @@ func (j *recordingJob) resume() error { return j.step("resume") } func (j *recordingJob) terminate() error { return j.step("terminate") } func (j *recordingJob) close() error { return j.step("close") } +func TestProcessJobCancellationBeforeActivationDoesNotResume(t *testing.T) { + j := &recordingJob{} + p := newProcessJob(j) + if err := p.stop(); err != nil { + t.Fatal(err) + } + if p.activate() == nil { + t.Fatal("canceled launch was resumed") + } + p.release() + if !reflect.DeepEqual(j.events, []string{"terminate", "close"}) { + t.Fatal(j.events) + } +} + func TestProcessJobAssignmentBeforeResume(t *testing.T) { j := &recordingJob{} p := newProcessJob(j) @@ -91,8 +106,8 @@ func TestProcessJobTerminationErrorStillCloses(t *testing.T) { if p.stop() == nil { t.Fatal("expected termination error") } - p.release() if !reflect.DeepEqual(j.events, []string{"terminate", "close"}) { - t.Fatal(j.events) + t.Fatal("termination error must close the job immediately", j.events) } + p.release() } diff --git a/services/nvpair-engine-manager/process_job_windows.go b/services/nvpair-engine-manager/process_job_windows.go index 6feb3d962..1024fa916 100644 --- a/services/nvpair-engine-manager/process_job_windows.go +++ b/services/nvpair-engine-manager/process_job_windows.go @@ -24,7 +24,7 @@ type windowsProcessJob struct { job, gate, process windows.Handle } -func configureProcessJob(cmd *exec.Cmd, breakaway bool) (*processJob, error) { +func configureProcessJob(cmd *exec.Cmd, detachedControl bool) (*processJob, error) { executable, err := os.Executable() if err != nil { return nil, err @@ -37,16 +37,22 @@ func configureProcessJob(cmd *exec.Cmd, breakaway bool) (*processJob, error) { owner := newProcessJob(native) limits := windows.JOBOBJECT_EXTENDED_LIMIT_INFORMATION{} limits.BasicLimitInformation.LimitFlags = windows.JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE - // Command-mode vendor CLIs can explicitly launch an independent daemon with - // CREATE_BREAKAWAY_FROM_JOB. Never enable SILENT_BREAKAWAY: ordinary children - // must remain owned, even when they reparent or detach their console. - if breakaway { - limits.BasicLimitInformation.LimitFlags |= windows.JOB_OBJECT_LIMIT_BREAKAWAY_OK + // Command-mode engines declare vendor-managed daemons, not owned server + // trees. Own the control CLI itself but preserve its independent children. + // Never apply this exception to servers, downloads, or install helpers. + if detachedControl { + limits.BasicLimitInformation.LimitFlags |= windows.JOB_OBJECT_LIMIT_SILENT_BREAKAWAY_OK } if _, err := windows.SetInformationJobObject(job, windows.JobObjectExtendedLimitInformation, uintptr(unsafe.Pointer(&limits)), uint32(unsafe.Sizeof(limits))); err != nil { owner.release() return nil, err } + if detachedControl { + // Suspend the actual CLI, not a launcher whose child would break away. + // The shared assign/resume sequence still gates all user code. + cmd.SysProcAttr = &syscall.SysProcAttr{HideWindow: true, CreationFlags: windows.CREATE_SUSPENDED | windows.CREATE_NO_WINDOW} + return owner, nil + } native.gate, err = windows.CreateEvent(nil, 1, 0, nil) if err != nil { owner.release() @@ -78,7 +84,36 @@ func (p *windowsProcessJob) assign() error { } return nil } -func (p *windowsProcessJob) resume() error { return windows.SetEvent(p.gate) } +func (p *windowsProcessJob) resume() error { + if p.gate != 0 { + return windows.SetEvent(p.gate) + } + // CREATE_SUSPENDED has not executed user code or spawned children. Both + // os.Process and p.process pin its identity while we open its initial thread. + snapshot, err := windows.CreateToolhelp32Snapshot(windows.TH32CS_SNAPTHREAD, 0) + if err != nil { + return err + } + defer windows.CloseHandle(snapshot) + entry := windows.ThreadEntry32{Size: uint32(unsafe.Sizeof(windows.ThreadEntry32{}))} + for err = windows.Thread32First(snapshot, &entry); err == nil; err = windows.Thread32Next(snapshot, &entry) { + if entry.OwnerProcessID != uint32(p.cmd.Process.Pid) { + continue + } + thread, err := windows.OpenThread(windows.THREAD_SUSPEND_RESUME|windows.THREAD_QUERY_LIMITED_INFORMATION, false, entry.ThreadID) + if err != nil { + return err + } + defer windows.CloseHandle(thread) + ownerPID, _, queryErr := windows.NewLazySystemDLL("kernel32.dll").NewProc("GetProcessIdOfThread").Call(uintptr(thread)) + if ownerPID != uintptr(p.cmd.Process.Pid) { + return fmt.Errorf("suspended thread identity changed: %w", queryErr) + } + _, err = windows.ResumeThread(thread) + return err + } + return fmt.Errorf("find suspended control thread: %w", err) +} func (p *windowsProcessJob) terminate() error { return windows.TerminateJobObject(p.job, 1) } func (p *windowsProcessJob) close() error { for _, h := range []windows.Handle{p.gate, p.process} { diff --git a/services/nvpair-engine-manager/testprocess_windows_test.go b/services/nvpair-engine-manager/testprocess_windows_test.go index 91be4f7d6..7b5bcb023 100644 --- a/services/nvpair-engine-manager/testprocess_windows_test.go +++ b/services/nvpair-engine-manager/testprocess_windows_test.go @@ -30,4 +30,6 @@ func testRunProcesses(root string) []fakeProcessRecord { return records } -func killTestProcessGroup(pid int) { _ = signalPID(pid, true) } +func signalPID(pid int, _ bool) error { killTestProcessGroup(pid); return nil } + +func killTestProcessGroup(pid int) { terminateOwnedPID(pid, 0, testProcessImage(pid)) } From d706132fae903560b924cd30077db58bcf24cdfc Mon Sep 17 00:00:00 2001 From: Jared Ping <13906915+JaredP94@users.noreply.github.com> Date: Fri, 6 Nov 2026 17:50:02 +0200 Subject: [PATCH 3/5] fix(engine-manager): pin legacy listener identity through stop Keep a query and terminate handle open from listener identification through orphan cleanup instead of reopening a PID after an image-only lookup. Record the process creation time and fail closed unless the same handle still has that identity and image and its PID still owns the managed listener. Route Windows test-process cleanup through the same pinned identity checks, including run ownership validation for copied fixtures. Preserve Unix orphan signal semantics. Add injected regressions for same-image PID replacements on another port and on the same port with a different creation time, plus fail-closed and handle-close coverage. Signed-off-by: Jared Ping <13906915+JaredP94@users.noreply.github.com> --- services/nvpair-engine-manager/README.md | 15 ++- .../engine_process_unix.go | 17 ++- .../nvpair-engine-manager/legacy_process.go | 69 ++++++++++ .../legacy_process_test.go | 121 ++++++++++++++++++ services/nvpair-engine-manager/lifecycle.go | 9 +- .../nvpair-engine-manager/proc_windows.go | 67 ++++++++-- .../testprocess_windows_test.go | 25 +++- 7 files changed, 300 insertions(+), 23 deletions(-) create mode 100644 services/nvpair-engine-manager/legacy_process.go create mode 100644 services/nvpair-engine-manager/legacy_process_test.go diff --git a/services/nvpair-engine-manager/README.md b/services/nvpair-engine-manager/README.md index 07ff26e87..b3a39a893 100644 --- a/services/nvpair-engine-manager/README.md +++ b/services/nvpair-engine-manager/README.md @@ -156,8 +156,8 @@ This does not extend the supported platform list. Each fake-engine executable (including test copies) has a per-test owner under the run's `nvpair-em-test-*` directory. Before doing any work, the fake records its PID and executable identity there; detached children inherit registration. -A `t.Cleanup` sweep force-kills owned process groups on Unix (process trees on -Windows) and fails the test if any remain. `TestMain` also checks registered +A `t.Cleanup` sweep force-kills owned process groups on Unix (individually +pinned, identity-checked processes on Windows) and fails the test if any remain. `TestMain` also checks registered children and processes whose executables are under this run's directory, sweeps survivors, and fails the run if a per-test cleanup was missed. Sweeps verify executable identity before signaling PIDs and never target other test runs. @@ -629,9 +629,14 @@ already-cancelled launch. No state mutex or job-handle mutex covers a process wait. Existing lifecycle-operation serialization remains unchanged. No `taskkill` calls remain. Legacy adopted Windows orphans have no job: reclaim -reopens and pins the process, revalidates its image against the already-verified -PAIR-owned install, then terminates **that handle only**. It does not enumerate -or kill descendants for which ownership cannot be proved. Foreign listeners and +opens a query/terminate handle when identifying the listener and records its +creation time and image. That handle stays open through termination; immediately +before terminating, reclaim checks the creation time and image through the same +handle, and re-reads the TCP listener table to confirm it still owns the managed +port. Any failed check declines termination. The pinned handle prevents PID reuse +between checks. Reclaim terminates **that handle only**, then closes it exactly +once on every path. It does not enumerate or kill descendants for which ownership +cannot be proved. Foreign listeners and vendor-managed daemon trees remain untouched. Windows runtime behavior is untested here; cross-compilation and injected-job diff --git a/services/nvpair-engine-manager/engine_process_unix.go b/services/nvpair-engine-manager/engine_process_unix.go index 3820a02c3..634ed6538 100644 --- a/services/nvpair-engine-manager/engine_process_unix.go +++ b/services/nvpair-engine-manager/engine_process_unix.go @@ -12,8 +12,23 @@ import ( "time" ) +type unixLegacyListener struct { + pid int + image string +} + +func findLegacyListener(port int) (legacyListener, bool) { + pid, image, ok := pidOnPort(port) + return &unixLegacyListener{pid: pid, image: image}, ok +} + +func (p *unixLegacyListener) processID() int { return p.pid } +func (p *unixLegacyListener) imagePath() string { return p.image } +func (p *unixLegacyListener) close() {} + // Preserve Unix orphan reclaim and its existing signal/deadline semantics. -func terminateOwnedPID(pid int, grace time.Duration, _ string) { +func (p *unixLegacyListener) terminate(grace time.Duration, _ string) { + pid := p.pid if pid <= 0 { return } diff --git a/services/nvpair-engine-manager/legacy_process.go b/services/nvpair-engine-manager/legacy_process.go new file mode 100644 index 000000000..909a2314a --- /dev/null +++ b/services/nvpair-engine-manager/legacy_process.go @@ -0,0 +1,69 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +package main + +import "time" + +// legacyListener owns the identity used for an adopted-process stop. Windows +// implementations retain a query/terminate handle until close; Unix retains +// its existing best-effort PID/group cleanup semantics. +type legacyListener interface { + processID() int + imagePath() string + terminate(time.Duration, string) + close() +} + +// pinnedProcessOperations always addresses one open kernel process object. +// ownsPort re-reads the listener table while that handle prevents PID reuse. +type pinnedProcessOperations interface { + imagePath() string + creationTime() (uint64, error) + ownsPort(port, pid int) bool + terminate(time.Duration) + close() +} + +type pinnedProcess struct { + pid, port int + image string + created uint64 + ops pinnedProcessOperations +} + +// identifyPinnedProcess takes ownership of ops, including on failure. A zero +// port identifies a non-listening helper; its caller must prove ownership by +// other means before asking to terminate it. +func identifyPinnedProcess(pid, port int, ops pinnedProcessOperations) (*pinnedProcess, bool) { + created, err := ops.creationTime() + image := ops.imagePath() + if err != nil || image == "" || (port != 0 && !ops.ownsPort(port, pid)) { + ops.close() + return nil, false + } + return &pinnedProcess{pid: pid, port: port, image: image, created: created, ops: ops}, true +} + +func (p *pinnedProcess) processID() int { return p.pid } +func (p *pinnedProcess) imagePath() string { return p.image } +func (p *pinnedProcess) close() { + if p.ops != nil { + p.ops.close() + p.ops = nil + } +} + +func (p *pinnedProcess) terminate(grace time.Duration, expectedImage string) { + if p.ops == nil { + return + } + created, err := p.ops.creationTime() + if err != nil || created != p.created || + !isOurEngineImage(p.ops.imagePath(), p.image) || + !isOurEngineImage(p.image, expectedImage) || + (p.port != 0 && !p.ops.ownsPort(p.port, p.pid)) { + return + } + p.ops.terminate(grace) +} diff --git a/services/nvpair-engine-manager/legacy_process_test.go b/services/nvpair-engine-manager/legacy_process_test.go new file mode 100644 index 000000000..91f69af09 --- /dev/null +++ b/services/nvpair-engine-manager/legacy_process_test.go @@ -0,0 +1,121 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +package main + +import ( + "errors" + "testing" + "time" +) + +// This injected kernel boundary models a pinned process object, not a PID +// lookup: changing its identity must never authorize terminating a replacement. +type fakePinnedProcess struct { + pid, port int + image string + created uint64 + queryErr error + terminated bool + closes int +} + +func (p *fakePinnedProcess) imagePath() string { + if p.closes != 0 { + return "" + } + return p.image +} +func (p *fakePinnedProcess) creationTime() (uint64, error) { + if p.closes != 0 { + return 0, errors.New("closed handle") + } + return p.created, p.queryErr +} +func (p *fakePinnedProcess) ownsPort(port, pid int) bool { return port == p.port && pid == p.pid } +func (p *fakePinnedProcess) terminate(time.Duration) { p.terminated = true } +func (p *fakePinnedProcess) close() { p.closes++ } + +func TestLegacyListenerRejectsSameImagePIDReplacement(t *testing.T) { + for _, replacement := range []struct { + name string + port int + created uint64 + }{ + {"another port", 11436, 20}, + {"same port different creation time", 11435, 20}, + } { + t.Run(replacement.name, func(t *testing.T) { + ops := &fakePinnedProcess{pid: 42, port: 11435, image: "/pair/engine", created: 10} + p, ok := identifyPinnedProcess(42, 11435, ops) + if !ok { + t.Fatal("original listener not identified") + } + ops.port, ops.created = replacement.port, replacement.created + p.terminate(0, "/pair/engine") + p.close() + if ops.terminated { + t.Fatal("terminated a same-image PID replacement") + } + if ops.closes != 1 { + t.Fatalf("handle closed %d times", ops.closes) + } + }) + } +} + +func TestLegacyListenerRevalidatesPinnedIdentity(t *testing.T) { + for _, failure := range []string{"none", "listener moved", "image changed", "creation query failed", "image query failed", "unexpected image"} { + t.Run(failure, func(t *testing.T) { + ops := &fakePinnedProcess{pid: 42, port: 11435, image: "/pair/engine", created: 10} + p, ok := identifyPinnedProcess(42, 11435, ops) + if !ok { + t.Fatal("listener not identified") + } + expected := "/pair/engine" + switch failure { + case "listener moved": + ops.port = 11436 + case "image changed": + ops.image = "/other/engine" + case "creation query failed": + ops.queryErr = errors.New("exited") + case "image query failed": + ops.image = "" + case "unexpected image": + expected = "/other/engine" + } + p.terminate(0, expected) + p.close() + p.close() + if ops.terminated != (failure == "none") { + t.Fatalf("termination=%v", ops.terminated) + } + if ops.closes != 1 { + t.Fatalf("handle closed %d times", ops.closes) + } + }) + } +} + +func TestLegacyListenerIdentificationFailureClosesHandle(t *testing.T) { + for _, failure := range []string{"creation query", "image query", "listener changed"} { + t.Run(failure, func(t *testing.T) { + ops := &fakePinnedProcess{pid: 42, port: 11435, image: "/pair/engine", created: 10} + switch failure { + case "creation query": + ops.queryErr = errors.New("denied") + case "image query": + ops.image = "" + case "listener changed": + ops.port = 11436 + } + if _, ok := identifyPinnedProcess(42, 11435, ops); ok { + t.Fatal("unproven listener identified") + } + if ops.terminated || ops.closes != 1 { + t.Fatalf("terminated=%v closes=%d", ops.terminated, ops.closes) + } + }) + } +} diff --git a/services/nvpair-engine-manager/lifecycle.go b/services/nvpair-engine-manager/lifecycle.go index e8a5375dd..e157b3b00 100644 --- a/services/nvpair-engine-manager/lifecycle.go +++ b/services/nvpair-engine-manager/lifecycle.go @@ -488,7 +488,12 @@ func (e *Executor) doStop(st *engineState, engine string) error { // desktop app on a different port (e.g. Ollama's own :11434 while // we manage :11435) is never touched; we decline only a process // whose image we can't confirm is ours. - pid, image, ok := pidOnPort(port) + listener, ok := findLegacyListener(port) + pid, image := 0, "" + if ok { + defer listener.close() + pid, image = listener.processID(), listener.imagePath() + } if ok && isOwnedInstallPath(binPath, st.installDir) && isOurEngineImage(image, binPath) { st.mu.Lock() st.stopping = true @@ -498,7 +503,7 @@ func (e *Executor) doStop(st *engineState, engine string) error { } st.mu.Unlock() grace := stopGrace(rt) - terminateOwnedPID(pid, grace, binPath) + listener.terminate(grace, binPath) if !pidAlive(pid) { e.markStopped(st, engine) return nil diff --git a/services/nvpair-engine-manager/proc_windows.go b/services/nvpair-engine-manager/proc_windows.go index ac9472ba8..d314ada53 100644 --- a/services/nvpair-engine-manager/proc_windows.go +++ b/services/nvpair-engine-manager/proc_windows.go @@ -61,7 +61,8 @@ type mibTCP6RowOwnerPID struct { // (IPv4 first, then IPv6) plus that process's full image path. ok is false // when nothing is listening or the owner can't be resolved, so a caller can // fail closed (decline the stop). It is precise to a single port: a genuine -// desktop app on a different port is never surfaced. +// desktop app on a different port is never surfaced. This observation-only +// lookup must not authorize termination; findLegacyListener retains identity. func pidOnPort(port int) (pid int, image string, ok bool) { if p, found := listenerPID(windows.AF_INET, port); found { return p, imagePathForPID(p), true @@ -139,27 +140,65 @@ func imagePathForPID(pid int) string { return windows.UTF16ToString(buf[:size]) } -// A legacy orphan has no job. Revalidate the expected PAIR-installed image -// through a pinned handle and terminate only that process object. Enumerating -// and killing a PID tree cannot establish ownership of its current children. -// The caller has already checked that expectedImage is in an owned install. -func terminateOwnedPID(pid int, grace time.Duration, expectedImage string) { - if pid <= 0 || expectedImage == "" { - return +// findLegacyListener opens the process once, at identification. Neither the +// identity checks nor termination reopen its PID. The second table read in +// identifyPinnedProcess rejects a listener that changed before OpenProcess. +func findLegacyListener(port int) (legacyListener, bool) { + pid, ok := listenerPID(windows.AF_INET, port) + if !ok { + pid, ok = listenerPID(windows.AF_INET6, port) + } + if !ok { + return nil, false + } + return openPinnedProcess(pid, port) +} + +func openPinnedProcess(pid, port int) (*pinnedProcess, bool) { + if pid <= 0 { + return nil, false } h, err := windows.OpenProcess(windows.SYNCHRONIZE|windows.PROCESS_QUERY_LIMITED_INFORMATION|windows.PROCESS_TERMINATE, false, uint32(pid)) if err != nil { - return + return nil, false } - defer windows.CloseHandle(h) + return identifyPinnedProcess(pid, port, &windowsPinnedProcess{handle: h}) +} + +type windowsPinnedProcess struct{ handle windows.Handle } + +func (p *windowsPinnedProcess) imagePath() string { buf := make([]uint16, 32768) size := uint32(len(buf)) - if windows.QueryFullProcessImageName(h, 0, &buf[0], &size) != nil || !isOurEngineImage(windows.UTF16ToString(buf[:size]), expectedImage) { - return + if windows.QueryFullProcessImageName(p.handle, 0, &buf[0], &size) != nil { + return "" } + return windows.UTF16ToString(buf[:size]) +} + +func (p *windowsPinnedProcess) creationTime() (uint64, error) { + var created, exited, kernel, user windows.Filetime + err := windows.GetProcessTimes(p.handle, &created, &exited, &kernel, &user) + return uint64(created.HighDateTime)<<32 | uint64(created.LowDateTime), err +} + +func (p *windowsPinnedProcess) ownsPort(port, pid int) bool { + for _, family := range []uint32{windows.AF_INET, windows.AF_INET6} { + if owner, ok := listenerPID(family, port); ok && owner == pid { + return true + } + } + return false +} + +func (p *windowsPinnedProcess) close() { _ = windows.CloseHandle(p.handle) } + +// A legacy orphan has no job. Terminate only the pinned, revalidated process +// object, never its descendants, whose ownership has not been established. +func (p *windowsPinnedProcess) terminate(grace time.Duration) { // Windowless legacy engines cannot receive WM_CLOSE. Unlike owned jobs, // there is no safe way to reclaim descendants without their own proof. - if windows.TerminateProcess(h, 1) != nil { + if windows.TerminateProcess(p.handle, 1) != nil { return } if grace <= 0 { @@ -167,7 +206,7 @@ func terminateOwnedPID(pid int, grace time.Duration, expectedImage string) { } deadline := time.Now().Add(grace) for time.Now().Before(deadline) { - status, err := windows.WaitForSingleObject(h, 100) + status, err := windows.WaitForSingleObject(p.handle, 100) if err != nil || status != uint32(windows.WAIT_TIMEOUT) { return } diff --git a/services/nvpair-engine-manager/testprocess_windows_test.go b/services/nvpair-engine-manager/testprocess_windows_test.go index 7b5bcb023..4206d63c7 100644 --- a/services/nvpair-engine-manager/testprocess_windows_test.go +++ b/services/nvpair-engine-manager/testprocess_windows_test.go @@ -4,7 +4,11 @@ package main import ( + "crypto/sha256" + "fmt" + "os" "os/exec" + "path/filepath" "strconv" "strings" ) @@ -32,4 +36,23 @@ func testRunProcesses(root string) []fakeProcessRecord { func signalPID(pid int, _ bool) error { killTestProcessGroup(pid); return nil } -func killTestProcessGroup(pid int) { terminateOwnedPID(pid, 0, testProcessImage(pid)) } +func killTestProcessGroup(pid int) { + // Identify and validate through the same handle, including for helpers + // without listeners. Never re-open a PID after an image-only lookup. + p, ok := openPinnedProcess(pid, 0) + if !ok { + return + } + defer p.close() + image := canonicalEnginePath(p.imagePath()) + if !isManagedInstallPath(image, canonicalEnginePath(testProcessRoot)) { + // Copied fixtures may live outside the run directory but must have + // been explicitly registered to an owner inside this test run. + key := fmt.Sprintf("%x", sha256.Sum256([]byte(image))) + owner, err := os.ReadFile(filepath.Join(testProcessRoot, "owners", key)) + if err != nil || !isManagedInstallPath(canonicalEnginePath(string(owner)), canonicalEnginePath(testProcessRoot)) { + return + } + } + p.terminate(0, p.imagePath()) +} From 0aa256cab1c2b36f6f9912856be4f8021aa0bf01 Mon Sep 17 00:00:00 2001 From: Jared Ping <13906915+JaredP94@users.noreply.github.com> Date: Fri, 6 Nov 2026 18:16:46 +0200 Subject: [PATCH 4/5] test(engine-manager): kill saved fixture PIDs only while still owned Review follow-up. Three test cleanups reopened a saved fixture PID with os.FindProcess and killed it unconditionally, so a PID the OS had since reused for another program could be killed. They now go through killOwnedTestPID, which kills only while the PID still runs that test's own fake-engine copy (a path unique to the test); on Windows the kill then revalidates ownership through a pinned handle. Signed-off-by: Jared Ping <13906915+JaredP94@users.noreply.github.com> --- .../download_completion_unix_test.go | 9 +++------ .../nvpair-engine-manager/download_shutdown_test.go | 13 ++++--------- services/nvpair-engine-manager/testprocess_test.go | 10 ++++++++++ 3 files changed, 17 insertions(+), 15 deletions(-) diff --git a/services/nvpair-engine-manager/download_completion_unix_test.go b/services/nvpair-engine-manager/download_completion_unix_test.go index ad8efc1b9..b6fcc329d 100644 --- a/services/nvpair-engine-manager/download_completion_unix_test.go +++ b/services/nvpair-engine-manager/download_completion_unix_test.go @@ -28,8 +28,9 @@ func TestLMSCompletionStopsDownloaderChildAfterLauncherExit(t *testing.T) { default: } }} + bin := fakeEngineForTest(t) go func() { - _, err := runLMSDownloadCommand(ctx, []string{fakeEngineForTest(t), "shutdown-download-fixture", marker, "exit-parent", childMarker}, output) + _, err := runLMSDownloadCommand(ctx, []string{bin, "shutdown-download-fixture", marker, "exit-parent", childMarker}, output) done <- err }() select { @@ -53,11 +54,7 @@ func TestLMSCompletionStopsDownloaderChildAfterLauncherExit(t *testing.T) { if err != nil { t.Fatal(err) } - t.Cleanup(func() { - if child, err := os.FindProcess(childPID); err == nil { - _ = child.Kill() - } - }) + t.Cleanup(func() { killOwnedTestPID(childPID, bin) }) address, err := os.ReadFile(childMarker) if err != nil { t.Fatal(err) diff --git a/services/nvpair-engine-manager/download_shutdown_test.go b/services/nvpair-engine-manager/download_shutdown_test.go index 0e99e84a5..f02109009 100644 --- a/services/nvpair-engine-manager/download_shutdown_test.go +++ b/services/nvpair-engine-manager/download_shutdown_test.go @@ -53,6 +53,7 @@ func testRunStopsDownloader(t *testing.T, ignore, midCancel, tree, exitEarly, us t.Fatal(err) } } + bin := fakeEngineForTest(t) marker := filepath.Join(t.TempDir(), "downloader-pid") childMarker := marker + "-child" events, unsub := ex.progress.subscribe("lmstudio") @@ -77,7 +78,7 @@ func testRunStopsDownloader(t *testing.T, ignore, midCancel, tree, exitEarly, us go func() { defer close(pullDone) _, _ = ex.trackedPull(ctx, "lmstudio", "fixture", "shutdown-pull", true, func(ctx context.Context) (json.RawMessage, error) { - argv := []string{fakeEngineForTest(t), "shutdown-download-fixture", marker, mode} + argv := []string{bin, "shutdown-download-fixture", marker, mode} if tree { argv = append(argv, childMarker) } @@ -99,9 +100,7 @@ func testRunStopsDownloader(t *testing.T, ignore, midCancel, tree, exitEarly, us t.Fatal(err) } t.Cleanup(func() { - if proc, err := os.FindProcess(pid); err == nil { - _ = proc.Kill() - } + killOwnedTestPID(pid, bin) cancel() select { case <-pullDone: @@ -130,11 +129,7 @@ func testRunStopsDownloader(t *testing.T, ignore, midCancel, tree, exitEarly, us if err != nil { t.Fatal(err) } - t.Cleanup(func() { - if proc, err := os.FindProcess(childPID); err == nil { - _ = proc.Kill() - } - }) + t.Cleanup(func() { killOwnedTestPID(childPID, bin) }) conn, err := net.DialTimeout("tcp", childAddress, time.Second) if err != nil { t.Fatal("child not alive before shutdown:", err) diff --git a/services/nvpair-engine-manager/testprocess_test.go b/services/nvpair-engine-manager/testprocess_test.go index ba04f46e2..d8da8c0a2 100644 --- a/services/nvpair-engine-manager/testprocess_test.go +++ b/services/nvpair-engine-manager/testprocess_test.go @@ -104,6 +104,16 @@ func trackedFakeProcesses(root string) []fakeProcessRecord { return alive } +// killOwnedTestPID kills a saved fixture PID only while it still runs this +// test's own fake-engine copy, whose path is unique to the test. A PID the OS +// has since reused for another program is left alone. On Windows, +// killTestProcessGroup then revalidates ownership through a pinned handle. +func killOwnedTestPID(pid int, bin string) { + if pid > 0 && pid != os.Getpid() && isOurEngineImage(testProcessImage(pid), bin) { + killTestProcessGroup(pid) + } +} + func sweepFakeProcesses(root string) []fakeProcessRecord { deadline := time.Now().Add(3 * time.Second) for { From ee14bb69eec21f7e5acb07c90427943a4e4356cb Mon Sep 17 00:00:00 2001 From: Jared Ping <13906915+JaredP94@users.noreply.github.com> Date: Fri, 6 Nov 2026 18:38:02 +0200 Subject: [PATCH 5/5] test(engine-manager): check ownership in the remaining saved-PID cleanups Review follow-up. Cleanups in the e2e shutdown, command-mode StopAll and command-stop tail tests signalled a PID after a background Wait could already have reaped it, or read it back from a file. They now use killOwnedTestPID with the test's own fake-engine copy. The remaining direct kills run before the test waits on its child, so the PID cannot have been reused. Signed-off-by: Jared Ping <13906915+JaredP94@users.noreply.github.com> --- services/nvpair-engine-manager/e2e_test.go | 9 +++------ services/nvpair-engine-manager/shutdown_tail_test.go | 10 ++++++---- services/nvpair-engine-manager/shutdown_test.go | 9 +++------ 3 files changed, 12 insertions(+), 16 deletions(-) diff --git a/services/nvpair-engine-manager/e2e_test.go b/services/nvpair-engine-manager/e2e_test.go index 533ae801c..c1f88fc8d 100644 --- a/services/nvpair-engine-manager/e2e_test.go +++ b/services/nvpair-engine-manager/e2e_test.go @@ -133,7 +133,8 @@ func TestE2EPrepareShutdownCancelsStartingEngine(t *testing.T) { cfg := t.TempDir() home := t.TempDir() pidFile := filepath.Join(t.TempDir(), "fake.pid") - manifest := testEngineManifest(fakeEngineForTest(t)) + bin := fakeEngineForTest(t) + manifest := testEngineManifest(bin) platform := manifest.Platforms[hostKey()] platform.Runtime.Env["FAKE_PID_FILE"] = pidFile platform.Runtime.Env["FAKE_START_DELAY"] = "1m" @@ -162,11 +163,7 @@ func TestE2EPrepareShutdownCancelsStartingEngine(t *testing.T) { if pid == 0 { t.Fatal("fake engine did not publish its PID") } - t.Cleanup(func() { - if pidAlive(pid) { - _ = signalPID(pid, true) - } - }) + t.Cleanup(func() { killOwnedTestPID(pid, bin) }) send(t, manager.stdin, 2, prepareShutdownMethod, nil) waitResult(t, manager.frames, "2", 10*time.Second) diff --git a/services/nvpair-engine-manager/shutdown_tail_test.go b/services/nvpair-engine-manager/shutdown_tail_test.go index 4afa64d02..8a8c65602 100644 --- a/services/nvpair-engine-manager/shutdown_tail_test.go +++ b/services/nvpair-engine-manager/shutdown_tail_test.go @@ -19,14 +19,15 @@ func TestCommandStopAndRestartWithPersistentHost(t *testing.T) { if err != nil { t.Fatal(err) } - cmd := exec.Command(fakeEngineForTest(t), "persistent-host", marker, "127.0.0.1:"+strconv.Itoa(port)) + bin := fakeEngineForTest(t) + cmd := exec.Command(bin, "persistent-host", marker, "127.0.0.1:"+strconv.Itoa(port)) configureSysProcAttr(cmd) if err := cmd.Start(); err != nil { t.Fatal(err) } done := make(chan struct{}) go func() { _ = cmd.Wait(); close(done) }() - t.Cleanup(func() { killTestProcessGroup(cmd.Process.Pid); <-done }) + t.Cleanup(func() { killOwnedTestPID(cmd.Process.Pid, bin); <-done }) waitPortServing(t, port) if pid, _, ok := pidOnPort(port); !ok || pid != cmd.Process.Pid { t.Skip("host cannot resolve listener PID") @@ -85,14 +86,15 @@ func testCommandStopTail(t *testing.T, delay string, grace int, wantTimeout bool if err != nil { t.Fatal(err) } - cmd := exec.Command(fakeEngineForTest(t), "shutdown-tail", marker, "127.0.0.1:"+strconv.Itoa(port), delay) + bin := fakeEngineForTest(t) + cmd := exec.Command(bin, "shutdown-tail", marker, "127.0.0.1:"+strconv.Itoa(port), delay) configureSysProcAttr(cmd) if err := cmd.Start(); err != nil { t.Fatal(err) } done := make(chan struct{}) go func() { _ = cmd.Wait(); close(done) }() - t.Cleanup(func() { _ = signalPID(cmd.Process.Pid, true); <-done }) + t.Cleanup(func() { killOwnedTestPID(cmd.Process.Pid, bin); <-done }) waitPortServing(t, port) if pid, _, ok := pidOnPort(port); !ok || pid != cmd.Process.Pid { t.Skip("host cannot resolve listener PID") diff --git a/services/nvpair-engine-manager/shutdown_test.go b/services/nvpair-engine-manager/shutdown_test.go index b393e6846..307a5b513 100644 --- a/services/nvpair-engine-manager/shutdown_test.go +++ b/services/nvpair-engine-manager/shutdown_test.go @@ -64,7 +64,8 @@ func TestStopAllCancelsCommandStartWithoutError(t *testing.T) { platform := manifest.Platforms[hostKey()] platform.Runtime.Mode = "command" platform.Runtime.Bin = "" - platform.Runtime.Start = [][]string{{fakeEngineForTest(t)}} + bin := fakeEngineForTest(t) + platform.Runtime.Start = [][]string{{bin}} platform.Runtime.Ready = nil platform.Runtime.Health = nil platform.Runtime.Stop = nil @@ -87,11 +88,7 @@ func TestStopAllCancelsCommandStartWithoutError(t *testing.T) { if pid == 0 { t.Fatal("command-mode fake engine did not publish its PID") } - t.Cleanup(func() { - if pidAlive(pid) { - _ = signalPID(pid, true) - } - }) + t.Cleanup(func() { killOwnedTestPID(pid, bin) }) ex.StopAll()