fix(engine-manager): Stopping a leftover Ollama engine works on macOS - #136

Follow-up to the orphan-reclaim fix, closing the one way it could silently
stop working in the shipped app.

procImage shelled out to a bare "lsof", which resolves through PATH. This
worker inherits whatever PATH the desktop app was launched with — nothing
between Electron, the broker and the worker sets one — and lsof lives in
/usr/sbin. A GUI launch normally has that on PATH, so this is not a
day-one break, but a user or launcher with a narrowed PATH would land back
on the original bug: reclaim refused, the same misleading "running under
external management" message, and nothing anywhere saying a tool was
missing. nvpair-node-info already addresses ioreg by absolute path for
exactly this reason.

The known macOS location is preferred and PATH is still consulted
otherwise, because this file also builds for the other BSDs, where lsof
comes from ports under a different prefix.

Also here, both documentation rather than behaviour:

The skip in spawnFakeListener described a host that cannot resolve a
port's owner as "no lsof/ss, or a /proc-less OS". The second half is no
longer true and was never harmless: it meant three orphan-reclaim tests
skipped on every macOS machine, reporting success on the one platform
where the behaviour they cover was broken. Worth stating plainly, since a
skip that looks routine is how this went unnoticed.

The engine-manager README described reclaim without mentioning that it
depends on an external tool at all, or that the check fails closed when
that tool is unavailable. CI already installs lsof and iproute2 for it.

Signed-off-by: Terve <[email protected]>

…H fallback

Review findings on the macOS reclaim fix.

**Windows could not build its own tests.** The new tests call `procImage`
from an untagged file, but Windows spelled the same concept
`imagePathForPID`, so `GOOS=windows go vet ./...` failed with "undefined:
procImage". `go build` passes because it excludes test files, which is why
my earlier cross-compile matrix missed it and why CI does too — the only
`go test` over services runs on Linux, and `services:windows-build` runs
`go build` alone. Windows now uses the same name as Linux and macOS, so the
concept has one spelling, the ownership check reads identically everywhere,
and a test that exercises it needs no build tag.

**macOS no longer consults PATH for lsof.** The previous commit preferred
the absolute path but still fell back to a PATH lookup, which re-opened the
hole its own comment argued had to be closed: this function's return value
authorizes terminatePID, so whichever binary answers it decides which
process gets killed. A test now plants an executable named `lsof` on PATH
that reports a PAIR-managed binary and asserts it is never consulted.

That fallback only ever existed for the BSDs, which are not supported, so
the file is now `procimage_darwin.go` and unsupported hosts get an explicit
`procimage_other.go` — matching the repo's existing `_other` convention
(`gpu_other.go`, `stats_other.go`). It returns "" and says plainly that
reclaim does not work there, rather than guessing.

**The engine-manager's own live tests still had the broken isolation** the
last commit fixed elsewhere: two variables, neither of which macOS reads,
while spawning a real manager against the developer's actual engines. They
also had not compiled since the repository's initial import — `configSubdir`
was referenced and never defined — so the gap was unreachable but would
have been inherited by anyone reviving them. Both are fixed, and the
isolation is now one helper the module shares rather than a block copied per
call site, so a new spawn site cannot get it half right. I could not execute
these tests: they need real Ollama and LM Studio installs behind
NVPAIR_LIVE_* gates. `go vet -tags live` now passes, which it did not before.

Also: a test pinning containment for a symlink that lives inside the install
directory but points outside it, since resolving both sides changed that
answer; and the parity doc no longer describes reclaim as unconditional.

Signed-off-by: Terve <[email protected]>

The module now has platform-exclusive source. procimage_darwin.go resolves
a PID's executable through lsof because macOS has no /proc, and it is what
decides whether stop and uninstall may terminate an adopted engine.

The `services` job is the only one that runs go test across services, and
it runs on Linux, where that file and its tests are excluded by build tag.
So the macOS implementation was compiled by the macOS build-script job and
otherwise never executed: every assertion covering the platform the fix
exists for ran nowhere. The reclaim tests that do run on Linux exercise the
/proc path instead, so nothing in CI touched the lsof one.

The macOS leg of the build-script matrix now runs this module's tests
natively after the build. The runner has lsof at /usr/sbin/lsof.

Signed-off-by: Terve <[email protected]>

Two review lenses independently found the same defect: the PATH hardening
was half applied, and my test certified a guarantee the system did not have.

pidOnPort resolves an owner in two steps — lsofPID finds which PID holds the
port, then procImage finds which executable that PID is running — and only
the second was given an absolute path. lsofPID runs first and gates
everything, so with /usr/sbin off PATH the lookup failed there, ssPID cannot
help on macOS, and the stop was declined exactly as before the branch.

The test made this worse rather than catching it: TestProcImageSurvivesAnEmptyPath
passed under a narrowed PATH while the two tests that actually exercise
reclaim skipped, because spawnFakeListener skips when the owner cannot be
resolved. A green suite reported a fix that was not there. Both halves are
on the same path to a kill, so the replacement test drives pidOnPort against
a real listener and fails if either half regresses. Verified: with
PATH=<go>:/usr/bin:/bin the reclaim tests now pass where they previously
skipped.

Resolution is now one helper used by all three call sites, with the ss
locations covered too, and it keeps a bare-name fallback only for
distributions that put these tools elsewhere — reaching it means no known
location exists, where a PATH lookup is the only remaining chance and
failing still declines rather than widens.

Also from the review:

lsof's exit status is not a statement about our PID. It returns 1 if it hit
any error anywhere — a filesystem it could not stat, routine with network or
FUSE mounts — while still printing correct records for the process asked
about. Treating that as total failure would blank the image and refuse a
legitimate stop, so output is now parsed whenever there is any, with -w to
suppress the warnings; only a failure to start the tool or a deadline
(possible truncation) is fatal.

The forced kill re-confirms the image. The identity check runs before the
graceful signal, and a PID freed during the grace period can be reused, so
the SIGKILL escalation — a process-group signal on Unix — could land on an
unrelated process. Newly reachable, since this path was dead on macOS.

resolveForCompare no longer cleans before resolving: filepath.Clean
collapses ".." lexically, which is wrong across a symlink and could make an
outside path look contained. installDir is resolved once when engineState is
built, so the containment guard and the {install_dir} an uninstall command
deletes cannot describe different directories. The lsof path parse trims only
a stray CR, since a path genuinely ending in a space would otherwise compare
equal to the managed binary.

Signed-off-by: Terve <[email protected]>

TestIPCTransport spawned a real manager with an inherited environment. The
manager resolves its engines and engine-bin directories through appdir at
startup, so it read the developer's actual installed engines — the same class
of leak this branch fixed elsewhere, and e2e_test.go in the same file set
already isolates for exactly this reason.

All four real-binary spawn sites in the module now go through one helper.

Signed-off-by: Terve <[email protected]>

…lookup

Second-pass review findings. The previous commit claimed both halves of the
owner lookup prefer absolute paths; that was true for lsof and not for the
rest of the path to a kill.

`ss` does not exist on macOS in any location, so `systemTool` fell through
to a bare-name PATH lookup there every time. Reaching it needed `lsofPID` to
fail first — and `lsofPID` still treated any non-zero lsof exit as "no
owner", which is exactly the condition the sibling `procImageVia` had just
been fixed for: lsof reports failure if it hit an error anywhere, including
a filesystem it could not stat, while printing the correct PID. So a routine
warning on a host with a network or FUSE mount dropped the lookup onto a
PATH-resolved tool that decides which PID gets signalled.

`ss` is iproute2 and Linux-only, so it now lives in a Linux-tagged file with
an explicit no-op elsewhere, rather than being looked up on a platform where
finding it at all would mean finding something PATH supplied. The lsof exit
handling is shared by both lookups now instead of existing in one of them.

The forced kill no longer signals a process group the ownership check never
examined. This branch is about adopted engines PAIR did not start, so the
group belongs to whoever launched them — a shell wrapper or a pipeline puts
unrelated processes in it. The group is signalled only when the target leads
it, which is guaranteed for anything PAIR started (configureSysProcAttr sets
Setpgid) and is what makes the escalation reach model runners; otherwise the
signal goes to the verified PID alone.

`systemTool` also checks a candidate is a regular executable file rather than
merely present, so a directory at one location cannot take the later
candidates out of play.

Three cross-process tests spawned a real broker with an inherited
environment. The broker loads and rewrites workloads-history.json through
appdir regardless of --cluster-dir, so those did not just read the
developer's data, they overwrote it. Two helpers in the same package already
did this correctly inline; the shared helper now makes that the default.
Verified by mtime: a six-minute full run of services/tests leaves both
workloads-history.json and lmstudio-proxy-port.json untouched.

Not taken, deliberately. The stop-decline error still names the offending
image path: it is documented, asserted by a test, and it is what tells the
operator which application to close. It can reach paired peers when Uninstall
wraps it, but the same class of path is already in a peer-visible error
pre-diff, nvpair-errors has no redaction at all, and the channel is
cluster-mTLS to paired members — so sanitizing is a change to that component,
not a line move here. Also left: manifest commands resolve argv[0] through
PATH, which is pre-existing and wider than anything here.

Signed-off-by: Terve <[email protected]>

Final review pass. No blockers; these are the remaining findings.

Four more children in services/tests ran against the developer's real PAIR
data. Two spawn a real engine-manager, which resolves its engines and
engine-bin directories through appdir — --cluster-dir does not cover those —
so they loaded real manifest overrides and detected real installs, and one
asserts on the resulting engine list. The other two are the log-level tests'
proxy spawns. The proxy opens no listener and reads no saved port until a
facade is enabled, which those tests never do, so they cannot collide with a
running PAIR; they get the same throwaway config directory anyway, so they do
not depend on that staying true. Every broker, engine-manager, and proxy
spawn in the package now isolates.

procImageVia had its own copy of runTool's exit-status handling. The two were
equivalent, but the rule about when a non-zero lsof exit still carries a
usable answer now lives in one place, so the two lookups on the path to a
kill cannot drift apart on it.

The install-directory documentation claimed more than the code does. state()
is usually first called before the engine is installed, when the directory
does not exist and resolution is a no-op, and the value is then cached — so
"the two always agree; they are the same value" was wrong. Correctness rests
on the guard resolving again at call time, which it does; the comment and
README now say that instead.

A symlinked install directory is followed to its target, which is deliberately
the opposite of the rule for a symlink one level down, and nothing pinned it.
Both directions now have a test: PAIR owns the install directory it was
configured with, so pointing engine-bin at a larger disk works, but it does
not own whatever a link inside that directory happens to reference.

Noted, not fixed: TestDataPlaneRefusesNonMemberWhenClustered failed once
under full-suite load (15s, against 1.2s in isolation) and passed on a repeat
run of the whole suite. It is in a file this branch does not touch and looks
load-sensitive rather than related.

Signed-off-by: Terve <[email protected]>

The README pointed at the macOS job by its former name. The engine manager's
tests now run natively on the macOS leg of the build-script job, while the
Linux services job still excludes the lsof path by build tag.

Signed-off-by: Terve <[email protected]>

TestProcImageRejectsInvalidPID looped over its PIDs and also asserted that
an empty image never matches. The PIDs are now named subtests through a
local helper, and the empty-image assertion is dropped here and from
TestProcImageFailsClosedOnAMissingTool: TestIsOurEngineImage already covers
it, so each of these tests now proves only its own half.

TestSystemToolPrefersAnAbsoluteLocation also checked the fallback to the
bare name. That is a separate resolution branch, so it has its own test.

Signed-off-by: Terve <[email protected]>

…parison

The OS reports the path the kernel recorded, which is already canonical.
Resolving it as well let a symlink created after exec change which
configured binary a running process appears to be. The configured binary
path and install directory are still resolved, which is what makes a
configured /var/... match the kernel's /private/var/... on macOS.

The symlink tests now prove one behavior each, and compare in the direction
production does: the canonical reported image against a configured path
through a symlink. TestProcImageResolvesThisProcess skips where procImage
has no implementation.

Signed-off-by: Terve <[email protected]>

…t commands

The install directory was resolved through symlinks and cached, and
uninstall substitutes it as {install_dir}. With <engine-bin>/<engine>
linked to an existing directory, the Linux rm -rf and Windows rmdir /s /q
deleted the directory the link pointed to instead of the link, and which
of the two happened depended on whether the link existed when the engine's
state was first built. The containment guard resolves the directory when it
compares, so the stored value does not need to be resolved.

Signed-off-by: Terve <[email protected]>

…p error

The error for a stop declined on a foreign listener reaches paired peers, in
the remote stop response and through nvpair-errors sync when uninstall wraps
it. It named the process's full image path, which can contain a local
username and, on macOS, is no longer empty. It now names the executable's
file name, which is what tells the operator which application to close, and
the full path is logged locally.

Signed-off-by: Terve <[email protected]>

Without a locale, which is the usual case for an app launched from Finder,
lsof escapes the bytes of a non-ASCII path, so an engine installed under a
directory such as /Users/jörg never matched its managed binary and reclaim
failed closed. The tools now run with LC_ALL=en_US.UTF-8.

Signed-off-by: Terve <[email protected]>

The -F output parse used bare "f", "ftxt" and "n" literals. They are named
constants now, and the procImage doc describes what the lookup decides
rather than how it was missing.

Signed-off-by: Terve <[email protected]>

…gnal

Neither kill-path safeguard had a test, so reverting either left the suite
green. terminatePID is now driven against a real process that ignores
SIGTERM, with stillOurs answering each way, and signalPID against a real
process group, targeting its leader and a member.

The comments now say what the checks guarantee and what they do not: the
re-check compares the image, so a recycled PID running the same binary
passes it, and group leadership does not show that PAIR started the group.

Signed-off-by: Terve <[email protected]>

The reclaim and decline tests skipped whenever the PID or image behind a port
could not be resolved, so a regression in that lookup on Linux passed CI by
skipping. On Linux, macOS and Windows it now fails; other systems still skip.

The bare-name fallback test moves to the Unix test file, where it also runs on
Linux, the one platform that can reach the fallback, and the systemTool
comment says why a PATH-found tool is acceptable there.

Signed-off-by: Terve <[email protected]>

TestMain now points the whole test process at a private config root, so the
per-child isolation no longer protects the developer's data; it keeps
children apart, for example the two engine managers the remote-engine tests
run as separate nodes. The helper comments say so. The TestMain variable
list and four hand-written copies of the same environment block now go
through configEnvKeys and configEnv, so a variable added later lands in all
of them.

Signed-off-by: Terve <[email protected]>

On Windows os.UserHomeDir reads USERPROFILE, and the LM Studio manifest
detects %USERPROFILE%\.lmstudio\bin\lms.exe. Neither isolation helper set
it, so a test running engine:get-installed could adopt the developer's real
LM Studio and later run lms server stop against it. Both helpers, and the
services/tests TestMain root, now set it.

Signed-off-by: Terve <[email protected]>

…lama

Isolating LOCALAPPDATA pointed the bundled detect path,
%LOCALAPPDATA%\Programs\Ollama\ollama.exe, at an empty temp directory, so
the spawn test reported Ollama as not installed. The test now resolves the
installed executable from its own environment and gives the isolated
manager an override manifest naming it.

Signed-off-by: Terve <[email protected]>

The README said both halves of the lookup shell out on every platform and
that a host without lsof cannot reclaim. Linux reads the image from /proc
and can find the PID with ss, and Windows uses native APIs, so the reclaim
section and the Cross-platform file list now say which platform uses what.
Code comments that still described an ss fallback on macOS, or narrated how
the macOS gap was found, describe current behavior instead.

Signed-off-by: Terve <[email protected]>

The tests ran as a step of the macOS build-script leg, so a failure was
reported as "Build script (macos-latest)" and shared that job's 20-minute
timeout with a native build of every binary. They now run in an
engine-manager-macos job with its own 15-minute timeout.

Signed-off-by: Terve <[email protected]>